netdev - Re: [PATCH net] filter: Account for tail adjustment during pull operations

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 PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

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

Date:   Tue, 13 Dec 2022 23:32:09 -0700
From:   "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@...cinc.com>
To:     Daniel Borkmann <daniel@...earbox.net>, <ast@...nel.org>,
        <andrii@...nel.org>, <martin.lau@...ux.dev>,
        <john.fastabend@...il.com>, <song@...nel.org>, <yhs@...com>,
        <kpsingh@...nel.org>, <sdf@...gle.com>, <haoluo@...gle.com>,
        <jolsa@...nel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
        <kuba@...nel.org>, <pabeni@...hat.com>, <bpf@...r.kernel.org>,
        <netdev@...r.kernel.org>
CC:     Sean Tranchetti <quic_stranche@...cinc.com>
Subject: Re: [PATCH net] filter: Account for tail adjustment during pull
 operations

On 12/13/2022 3:42 PM, Daniel Borkmann wrote:
> On 12/13/22 5:39 AM, Subash Abhinov Kasiviswanathan wrote:
>> Extending the tail can have some unexpected side effects if a program is
>> reading the content beyond the head skb headlen and all the skbs in the
>> gso frag_list are linear with no head_frag -
>>
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index bb0136e..d5f7f79 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -1654,6 +1654,20 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, 
>> bpf_sp);
>>   static inline int __bpf_try_make_writable(struct sk_buff *skb,
>>                         unsigned int write_len)
>>   {
>> +    struct sk_buff *list_skb = skb_shinfo(skb)->frag_list;
>> +
>> +    if (skb_is_gso(skb) && list_skb && !list_skb->head_frag &&
>> +        skb_headlen(list_skb)) {
>> +        int headlen = skb_headlen(skb);
>> +        int err = skb_ensure_writable(skb, write_len);
>> +
>> +        /* pskb_pull_tail() has occurred */
>> +        if (!err && headlen != skb_headlen(skb))
>> +            skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
>> +
>> +        return err;
>> +    }
> 
> __bpf_try_make_writable() does not look like the right location to me
> given this is called also from various other places. bpf_skb_change_tail
> has skb_gso_reset in there, potentially that or pskb_pull_tail itself
> should mark it?

Actually the program we used had BPF_FUNC_skb_pull_data and we put this 
check in __bpf_try_make_writable so that it would help out 
BPF_FUNC_skb_pull_data & other users of __bpf_try_make_writable. Having 
the check in __pskb_pull_tail seems preferable though. Could you tell if 
the following is acceptable as this works for us -

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index dfc14a7..0f60abb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2263,6 +2263,9 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta)
                                 insp = list;
                         } else {
                                 /* Eaten partially. */
+                               if (skb_is_gso(skb) && !list->head_frag &&
+                                   skb_headlen(list))
+                                       skb_shinfo(skb)->gso_type |= 
SKB_GSO_DODGY;

                                 if (skb_shared(list)) {
                                         /* Sucks! We need to fork list. 
:-( */

> 
>>       return skb_ensure_writable(skb, write_len);
>>   }
>>
>