netdev - Re: [PATCH v2 net] udp: on peeking bad csum, drop packets even if not at head

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Aug 2017 13:29:34 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 net] udp: on peeking bad csum, drop packets even if not
 at head

On Tue, Aug 22, 2017 at 12:47 PM, Paolo Abeni <pabeni@...hat.com> wrote:
> On Tue, 2017-08-22 at 09:39 -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@...gle.com>
>>
>> When peeking, if a bad csum is discovered, the skb is unlinked from
>> the queue with __sk_queue_drop_skb and the peek operation restarted.
>>
>> __sk_queue_drop_skb only drops packets that match the queue head.
>>
>> This fails if the skb was found after the head, using SO_PEEK_OFF
>> socket option. This causes an infinite loop.
>>
>> We MUST drop this problematic skb, and we can simply check if skb was
>> already removed by another thread, by looking at skb->next :
>>
>> This pointer is set to NULL by the  __skb_unlink() operation, that might
>> have happened only under the spinlock protection.
>>
>> Many thanks to syzkaller team (and particularly Dmitry Vyukov who
>> provided us nice C reproducers exhibiting the lockup) and Willem de
>> Bruijn who provided first version for this patch and a test program.
>>
>> Fixes: 627d2d6b5500 ("udp: enable MSG_PEEK at non-zero offset")
>> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
>> Cc: Willem de Bruijn <willemb@...gle.com>
>> ---
>>  net/core/datagram.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/datagram.c b/net/core/datagram.c
>> index a21ca8dee5ea..8c2f4489ff8f 100644
>> --- a/net/core/datagram.c
>> +++ b/net/core/datagram.c
>> @@ -362,7 +362,7 @@ int __sk_queue_drop_skb(struct sock *sk, struct sk_buff_head *sk_queue,
>>       if (flags & MSG_PEEK) {
>>               err = -ENOENT;
>>               spin_lock_bh(&sk_queue->lock);
>> -             if (skb == skb_peek(sk_queue)) {
>> +             if (skb->next) {
>>                       __skb_unlink(skb, sk_queue);
>>                       refcount_dec(&skb->users);
>>                       if (destructor)
>>
>
> This version is really nice!

It is :) Thanks, Eric!

Acked-by: Willem de Bruijn <willemb@...gle.com>