Re: use-after-free in sctp_do_sm

[Joe Perches <joe@perches.com>]

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