netdev - Re: [PATCH net 2/2] net: add more sanity checks to qdisc_pkt_len

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: <e7afc629-5428-46d8-8db0-07d3588eb2c9@redhat.com>
Date: Tue, 1 Oct 2024 11:57:40 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>,
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, David Ahern <dsahern@...nel.org>, netdev@...r.kernel.org,
 Willem de Bruijn <willemb@...gle.com>,
 Jonathan Davies <jonathan.davies@...anix.com>, eric.dumazet@...il.com
Subject: Re: [PATCH net 2/2] net: add more sanity checks to
 qdisc_pkt_len_init()

On 9/26/24 11:19, Eric Dumazet wrote:
> On Thu, Sep 26, 2024 at 11:17 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
>>
>> Willem de Bruijn wrote:
>>> Eric Dumazet wrote:
>>>> One path takes care of SKB_GSO_DODGY, assuming
>>>> skb->len is bigger than hdr_len.
>>>>
>>>> virtio_net_hdr_to_skb() does not fully dissect TCP headers,
>>>> it only make sure it is at least 20 bytes.
>>>>
>>>> It is possible for an user to provide a malicious 'GSO' packet,
>>>> total length of 80 bytes.
>>>>
>>>> - 20 bytes of IPv4 header
>>>> - 60 bytes TCP header
>>>> - a small gso_size like 8
>>>>
>>>> virtio_net_hdr_to_skb() would declare this packet as a normal
>>>> GSO packet, because it would see 40 bytes of payload,
>>>> bigger than gso_size.
>>>>
>>>> We need to make detect this case to not underflow
>>>> qdisc_skb_cb(skb)->pkt_len.
>>>>
>>>> Fixes: 1def9238d4aa ("net_sched: more precise pkt_len computation")
>>>> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>>>> ---
>>>>   net/core/dev.c | 10 +++++++---
>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index f2c47da79f17d5ebe6b334b63d66c84c84c519fc..35b8bcfb209bd274c81380eaf6e445641306b018 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -3766,10 +3766,14 @@ static void qdisc_pkt_len_init(struct sk_buff *skb)
>>>>                              hdr_len += sizeof(struct udphdr);
>>>>              }
>>>>
>>>> -           if (shinfo->gso_type & SKB_GSO_DODGY)
>>>> -                   gso_segs = DIV_ROUND_UP(skb->len - hdr_len,
>>>> -                                           shinfo->gso_size);
>>>> +           if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) {
>>>> +                   int payload = skb->len - hdr_len;
>>>>
>>>> +                   /* Malicious packet. */
>>>> +                   if (payload <= 0)
>>>> +                           return;
>>>> +                   gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size);
>>>> +           }
>>>
>>> Especially for a malicious packet, should gso_segs be reinitialized to
>>> a sane value? As sane as feasible when other fields cannot be fully
>>> trusted..
>>
>> Never mind. I guess the best thing we can do is to leave pkt_len as
>> skb->len, indeed.
> 
> It is unclear if we can change a field in skb here, I hope that in the
> future we can make virtio_net_hdr_to_skb() safer.

A problem with virtio_net_hdr_to_skb() is that is needs to cope with 
existing implementation bending the virtio spec to its limits, and the 
spec definition allowed for some 'fun'. Sanitize everything is likely to 
break existing users.

On a somewhat related note, I'm trying to push a new virtio-spec GSO 
feature (UDP tunnel support) that will inevitably increase the attack 
surface. On the flip side the general idea is to be as strict as 
possible with the definition of the new allowed gso types, to try to 
avoid the above kind of scenarios (for such gso types).

The above just FYI,

Paolo