netdev - Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

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]

Message-ID: <CAF=yD-JH8ahoLNKOVjBScRXKP4UQqQpfq89C6xq0=nwd3jQtzw@mail.gmail.com>
Date:   Fri, 11 May 2018 19:16:14 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Sean Tranchetti <stranche@...eaurora.org>,
        Willem de Bruijn <willemb@...gle.com>,
        David Miller <davem@...emloft.net>,
        Network Development <netdev@...r.kernel.org>,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Subject: Re: [PATCH net-next] udp: Fix kernel panic in UDP GSO path

On Thu, May 10, 2018 at 8:51 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>
>
> On 05/10/2018 05:38 PM, Sean Tranchetti wrote:
>> Using GSO in the UDP path on a device with
>> scatter-gather netdevice feature disabled will result in a kernel
>> panic with the following call stack:
>>
>> This panic is the result of allocating SKBs with small size
>> for the newly segmented SKB. If the scatter-gather feature is
>> disabled, the code attempts to call skb_put() on the small SKB
>> with an argument of nearly the entire unsegmented SKB length.
>>
>> After this patch, attempting to use GSO with scatter-gather
>> disabled will result in -EINVAL being returned.
>>
>> Fixes: 15e36f5b8e98 ("udp: paged allocation with gso")
>> Signed-off-by: Sean Tranchetti <stranche@...eaurora.org>
>> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
>> ---
>>  net/ipv4/ip_output.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
>> index b5e21eb..0d63690 100644
>> --- a/net/ipv4/ip_output.c
>> +++ b/net/ipv4/ip_output.c
>> @@ -1054,8 +1054,16 @@ static int __ip_append_data(struct sock *sk,
>>                       copy = length;
>>
>>               if (!(rt->dst.dev->features&NETIF_F_SG)) {
>> +                     struct sk_buff *tmp;
>>                       unsigned int off;
>>
>> +                     if (paged) {
>> +                             err = -EINVAL;
>> +                             while ((tmp = __skb_dequeue(queue)) != NULL)
>> +                                     kfree(tmp);
>> +                             goto error;
>> +                     }
>> +
>>                       off = skb->len;
>>                       if (getfrag(from, skb_put(skb, copy),
>>                                       offset, copy, off, skb) < 0) {
>>
>
>
> Hmm, no, we absolutely need to fix GSO instead.
>
> Think of a bonding device (or any virtual devices), your patch wont avoid the crash.

Thanks for reporting the issue.

Paged skbuffs is an optimization for gso, but the feature should
continue to work even if gso skbs are linear, indeed (if at the cost
of copying during skb_segment).

We need to make paged contingent on scatter-gather. Rough
patch below. That is for ipv4 only, the same will be needed for ipv6.

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b5e21eb198d8..b38731d8a44f 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -884,7 +884,7 @@ static int __ip_append_data(struct sock *sk,

        exthdrlen = !skb ? rt->dst.header_len : 0;
        mtu = cork->gso_size ? IP_MAX_MTU : cork->fragsize;
-       paged = !!cork->gso_size;
+       paged = cork->gso_size && (rt->dst.dev->features & NETIF_F_SG);