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:42:36 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Subash Abhinov Kasiviswanathan <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/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 -
> 
>    kernel BUG at net/core/skbuff.c:4219!
>    pc : skb_segment+0xcf4/0xd2c
>    lr : skb_segment+0x63c/0xd2c
>    Call trace:
>     skb_segment+0xcf4/0xd2c
>     __udp_gso_segment+0xa4/0x544
>     udp4_ufo_fragment+0x184/0x1c0
>     inet_gso_segment+0x16c/0x3a4
>     skb_mac_gso_segment+0xd4/0x1b0
>     __skb_gso_segment+0xcc/0x12c
>     udp_rcv_segment+0x54/0x16c
>     udp_queue_rcv_skb+0x78/0x144
>     udp_unicast_rcv_skb+0x8c/0xa4
>     __udp4_lib_rcv+0x490/0x68c
>     udp_rcv+0x20/0x30
>     ip_protocol_deliver_rcu+0x1b0/0x33c
>     ip_local_deliver+0xd8/0x1f0
>     ip_rcv+0x98/0x1a4
>     deliver_ptype_list_skb+0x98/0x1ec
>     __netif_receive_skb_core+0x978/0xc60
> 
> Fix this by marking these skbs as GSO_DODGY so segmentation can handle
> the tail updates accordingly.
> 
> Fixes: 5293efe62df8 ("bpf: add bpf_skb_change_tail helper")
> Signed-off-by: Sean Tranchetti <quic_stranche@...cinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>
> ---
>   net/core/filter.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> 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?

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