On Thu, Dec 3, 2015 at 6:02 PM, Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> On Thu, Dec 3, 2015 at 7:55 AM, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>> On Thu, Dec 3, 2015 at 3:48 PM, Eric Dumazet <edumazet@xxxxxxxxxx> 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...
>>
>> 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 <sctp_do_sm+0x176>
>> 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 <sctp_id2assoc>
>> 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
>
> #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.
>
> 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.
FWIW I enabled CONFIG_DYNAMIC_DEBUG on my fuzzer. Not that it gives
any particular guarantees, but still can catch some of these.
> 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 <eric_test_pr_debug>:
> 4740: e8 00 00 00 00 callq 4745 <eric_test_pr_debug+0x5>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
