From: Joe Perches <joe@perches•com>
To: Aaron Conole <aconole@redhat•com>,
Dmitry Vyukov <dvyukov@google•com>,
Andrew Morton <akpm@linux-foundation•org>
Cc: Eric Dumazet <edumazet@google•com>,
syzkaller <syzkaller@googlegroups•com>,
Vladislav Yasevich <vyasevich@gmail•com>,
linux-sctp@vger•kernel.org, netdev <netdev@vger•kernel.org>,
Kostya Serebryany <kcc@google•com>,
Alexander Potapenko <glider@google•com>,
Sasha Levin <sasha.levin@oracle•com>
Subject: Re: use-after-free in sctp_do_sm
Date: Thu, 03 Dec 2015 11:06:59 -0800 [thread overview]
Message-ID: <1449169619.32567.8.camel@perches.com> (raw)
In-Reply-To: <f7twpsvgyar.fsf@aconole.bos.csb>
On Thu, 2015-12-03 at 13:52 -0500, Aaron Conole wrote:
> Dmitry Vyukov <dvyukov@google•com> writes:
> > On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet <edumazet@google•com> wrote:
> > > On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov <dvyukov@google•com> wrote:
> > > > On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet wrote:
> > > > > >
> > > > > > No, I don't. But pr_debug always computes its arguments. See no_printk
> > > > > > in printk.h. So this use-after-free happens for all users.
> > > > >
> > > > > Hmm.
> > > > >
> > > > > pr_debug() should be a nop unless either DEBUG or
> > > > > CONFIG_DYNAMIC_DEBUG are set
> > > > >
> > > > > On our production kernels, pr_debug() is a nop.
> > > > >
> > > > > Can you double check ? Thanks !
> > > >
> > > >
> > > > Why should it be nop? no_printk thing in printk.h pretty much
> > > > explicitly makes it not a nop...
>
> Because it was until commit 5264f2f75d8. It also violates my reading of
> the following from printk.h:
>
> * All of these will print unconditionally, although note that pr_debug()
> * and other debug macros are compiled out unless either DEBUG is defined
> * or CONFIG_DYNAMIC_DEBUG is set.
>
> > > >
> > > > Double-checked: debug_post_sfx leads to some generated code:
> > > >
> > > > debug_post_sfx();
> > > > ffffffff8229f256: 48 8b 85 58 fe ff ff mov -0x1a8(%rbp),%rax
> > > > ffffffff8229f25d: 48 85 c0 test %rax,%rax
> > > > ffffffff8229f260: 74 24 je
> > > > ffffffff8229f286
> > > > ffffffff8229f262: 8b b0 a8 00 00 00 mov 0xa8(%rax),%esi
> > > > ffffffff8229f268: 48 8b 85 60 fe ff ff mov -0x1a0(%rbp),%rax
> > > > ffffffff8229f26f: 44 89 85 74 fe ff ff mov %r8d,-0x18c(%rbp)
> > > > ffffffff8229f276: 48 8b 78 20 mov 0x20(%rax),%rdi
> > > > ffffffff8229f27a: e8 71 28 01 00 callq
> > > > ffffffff822b1af0
> > > > ffffffff8229f27f: 44 8b 85 74 fe ff ff mov -0x18c(%rbp),%r8d
> > > >
> > > > return error;
> > > > }
> > > > ffffffff8229f286: 48 81 c4 a0 01 00 00 add $0x1a0,%rsp
> > > > ffffffff8229f28d: 44 89 c0 mov %r8d,%eax
> > > > ffffffff8229f290: 5b pop %rbx
> > > > ffffffff8229f291: 41 5c pop %r12
> > > > ffffffff8229f293: 41 5d pop %r13
> > > > ffffffff8229f295: 41 5e pop %r14
> > > > ffffffff8229f297: 41 5f pop %r15
> > > > ffffffff8229f299: 5d pop %rbp
> > > > ffffffff8229f29a: c3 retq
> > >
> > > This is a serious concern, because we let in the past lot of patches
> > > converting traditional
>
> +1
>
> > > #ifdef DEBUG
> > > # define some_hand_coded_ugly_debug() printk( ...._
> > > #else
> > > # define some_hand_coded_ugly_debug()
> > > #endif
> > >
> > > On the premise pr_debug() would be a nop.
> > >
> > > It seems it is not always the case. This is a very serious problem.
>
> +1
>
> > > We probably have hundred of potential bugs, because few people
> > > actually make sure all debugging stuff is correct,
> > > like comments can be wrong because they are not updated properly as
> > > time flies.
> > >
> > > It is definitely a nop for many cases.
> > >
> > > +void eric_test_pr_debug(struct sock *sk)
> > > +{
> > > + if (atomic_read(&sk->sk_omem_alloc))
> > > + pr_debug("%s: optmem leakage for sock %p\n",
> > > + __func__, sk);
> > > +}
> > >
> > > ->
> > >
> > > 0000000000004740 :
> > > 4740: e8 00 00 00 00 callq 4745
> > > 4741: R_X86_64_PC32 __fentry__-0x4
> > > 4745: 55 push %rbp
> > > 4746: 8b 87 24 01 00 00 mov 0x124(%rdi),%eax //
> > > atomic_read() but nothing follows
> > > 474c: 48 89 e5 mov %rsp,%rbp
> > > 474f: 5d pop %rbp
> > > 4750: c3 retq
> >
> >
> >
> > I would expect that it is nop when argument evaluation does not have
> > side-effects. For example, for a load of a variable compiler will most
> > likely elide it (though, it does not have to elide it, because the
> > load is spelled in the code, so it can also legally emit the load and
> > doesn't use the result).
> > But if argument computation has side-effect (or compiler can't prove
> > otherwise), it must emit code. It must emit code for function calls
> > when the function is defined in a different translation unit, and for
> > volatile accesses (most likely including atomic accesses), etc
>
> This isn't 100% true. As you state, in order to reach the return 0, all
> side effects must be evaluated. Load generally does not have side
> effects, so it can be safely elided, but function() must be emitted.
>
> However, that is _not_ required to get the desired warning emission on a
> printf argument function, see http://pastebin.com/UHuaydkj for an
> example.
>
> I think that as a minimum, the following patch should be evaluted, but am
> unsure to whom I should submit it (after I test):
Andrew Morton <akpm@linux-foundation•org> (cc'd)
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 9729565..cd24d2d 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -286,7 +286,7 @@ extern asmlinkage void dump_stack(void) __cold;
> printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> #else
> #define pr_debug(fmt, ...) \
> - no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> + ({ if(0) printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); 0;})
More common is to use do {} while (0) instead of a
statement expression.
I think it'd be good to change pr_debug and variants to
do { if (0) no_printk(...) } while (0)
or some other form that completely eliminates all the
side-effects/function evaluations.
I think the same should be true when CONFIG_PRINTK is
not enabled.
https://lkml.org/lkml/2014/12/3/696
next prev parent reply other threads:[~2015-12-03 19:07 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-24 9:15 use-after-free in sctp_do_sm Dmitry Vyukov
2015-11-24 9:31 ` Dmitry Vyukov
2015-11-24 10:10 ` Dmitry Vyukov
2015-11-24 20:45 ` Neil Horman
2015-11-24 21:08 ` Eric Dumazet
2015-11-24 21:12 ` David Miller
2015-11-25 15:12 ` Vlad Yasevich
2015-11-28 15:50 ` Dmitry Vyukov
2015-12-03 16:51 ` Marcelo Ricardo Leitner
2015-12-03 17:43 ` Marcelo Ricardo Leitner
2015-12-03 17:59 ` Eric Dumazet
2015-12-03 18:06 ` Marcelo
2015-12-03 18:35 ` Vlad Yasevich
2015-12-03 18:43 ` Marcelo
2015-12-04 17:14 ` [PATCH net 0/3] sctp: packet timestamp fixes Marcelo Ricardo Leitner
2015-12-04 17:14 ` [PATCH net 1/3] sctp: use the same clock as if sock source timestamps were on Marcelo Ricardo Leitner
2015-12-04 20:31 ` Vlad Yasevich
2015-12-04 17:14 ` [PATCH net 2/3] sctp: update the netstamp_needed counter when copying sockets Marcelo Ricardo Leitner
2015-12-04 20:33 ` Vlad Yasevich
2015-12-04 17:14 ` [PATCH net 3/3] sctp: also copy sk_tsflags when copying the socket Marcelo Ricardo Leitner
2015-12-04 20:33 ` Vlad Yasevich
2015-12-06 3:24 ` [PATCH net 0/3] sctp: packet timestamp fixes David Miller
2015-12-03 13:05 ` use-after-free in sctp_do_sm Marcelo Ricardo Leitner
2015-12-03 13:45 ` Dmitry Vyukov
2015-12-03 14:48 ` Eric Dumazet
2015-12-03 15:55 ` Dmitry Vyukov
2015-12-03 16:15 ` Marcelo Ricardo Leitner
2015-12-03 17:02 ` Eric Dumazet
2015-12-03 17:12 ` Dmitry Vyukov
2015-12-03 18:52 ` Aaron Conole
2015-12-03 19:06 ` Joe Perches [this message]
2015-12-03 19:32 ` Jason Baron
2015-12-03 20:03 ` Joe Perches
2015-12-03 20:10 ` Jason Baron
2015-12-03 20:24 ` Joe Perches
2015-12-03 20:42 ` Jason Baron
2015-12-03 20:51 ` Joe Perches
2015-12-04 10:40 ` Dmitry Vyukov
2015-12-04 12:55 ` Marcelo Ricardo Leitner
2015-12-04 15:37 ` Vlad Yasevich
2015-12-04 15:51 ` Aaron Conole
2015-12-04 16:12 ` Dmitry Vyukov
2015-12-04 16:47 ` Jason Baron
2015-12-04 17:03 ` Joe Perches
2015-12-04 17:11 ` Jason Baron
2015-12-04 10:41 ` Dmitry Vyukov
2015-12-04 17:48 ` Marcelo Ricardo Leitner
2015-12-04 20:25 ` Dmitry Vyukov
2015-12-04 21:34 ` Marcelo Ricardo Leitner
2015-12-04 21:38 ` Dmitry Vyukov
2015-12-05 16:39 ` Vlad Yasevich
2015-12-07 11:26 ` Dmitry Vyukov
2015-12-07 13:15 ` Marcelo Ricardo Leitner
2015-12-07 13:20 ` Dmitry Vyukov
2015-12-07 18:52 ` Marcelo Ricardo Leitner
2015-12-07 19:33 ` Vlad Yasevich
2015-12-07 19:50 ` Marcelo Ricardo Leitner
2015-12-07 20:37 ` Vlad Yasevich
2015-12-07 20:52 ` Marcelo Ricardo Leitner
2015-12-08 17:30 ` Dmitry Vyukov
2015-12-08 17:40 ` Marcelo Ricardo Leitner
2015-12-08 19:22 ` Dmitry Vyukov
2015-12-09 14:41 ` Dmitry Vyukov
2015-12-09 15:03 ` Marcelo Ricardo Leitner
2015-12-09 16:41 ` Marcelo Ricardo Leitner
2015-12-11 13:35 ` Dmitry Vyukov
2015-12-11 13:51 ` Marcelo Ricardo Leitner
2015-12-11 14:03 ` Marcelo Ricardo Leitner
2015-12-11 14:30 ` Dmitry Vyukov
2015-12-11 15:55 ` Marcelo Ricardo Leitner
2016-01-08 13:00 ` [PATCH] sctp: fix use-after-free in pr_debug statement Marcelo Ricardo Leitner
2016-01-11 17:00 ` Vlad Yasevich
2016-01-11 22:13 ` David Miller
2016-01-12 8:41 ` Dmitry Vyukov
2015-12-11 18:37 ` use-after-free in sctp_do_sm Vlad Yasevich
2015-12-14 9:50 ` David Laight
2015-12-14 14:25 ` Vlad Yasevich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1449169619.32567.8.camel@perches.com \
--to=joe@perches$(echo .)com \
--cc=aconole@redhat$(echo .)com \
--cc=akpm@linux-foundation$(echo .)org \
--cc=dvyukov@google$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=glider@google$(echo .)com \
--cc=kcc@google$(echo .)com \
--cc=linux-sctp@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sasha.levin@oracle$(echo .)com \
--cc=syzkaller@googlegroups$(echo .)com \
--cc=vyasevich@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox