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 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] [day] [month] [year] [list]

Message-ID: <7bacb02c-659c-7921-a15d-8c758bb49156@iogearbox.net>
Date:   Thu, 15 Dec 2022 00:23:39 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@...cinc.com>,
        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/14/22 7:32 AM, Subash Abhinov Kasiviswanathan (KS) wrote:
> 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 -

Ah okay, that is good to know. The Fixes tag might have been misleading in that
case. From what you describe it sounds like a generic __pskb_pull_tail() issue
then? If so I'd go with the below for -net tree as a generic fix, yes.

> 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);
>>>   }
>>>
>>