netdev - Re: [PATCH net v2] skbuff: 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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 15 Dec 2022 09:29:05 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, shmulik@...anetworks.com, willemb@...gle.com,
        alexanderduyck@...com, netdev@...r.kernel.org, daniel@...earbox.net
Cc:     Sean Tranchetti <quic_stranche@...cinc.com>
Subject: Re: [PATCH net v2] skbuff: Account for tail adjustment during pull
 operations

On Wed, 2022-12-14 at 23:11 -0700, Subash Abhinov Kasiviswanathan
wrote:
> Extending the tail can have some unexpected side effects if a program uses
> a helper like BPF_FUNC_skb_pull_data to read partial content beyond the
> head skb headlen when 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: 3dcbdb134f32 ("net: gso: Fix skb_segment splat when splitting gso_size mangled skb having linear-headed frag_list")
> Signed-off-by: Sean Tranchetti <quic_stranche@...cinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@...cinc.com>
> ---
> v2: Fix the issue in __pskb_pull_tail() instead of __bpf_try_make_writable()
> as the issue is generic as mentioned by Daniel. Update the commit text
> and Fixes tag accordingly.
> 
>  net/core/skbuff.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 88fa405..759bede 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2416,6 +2416,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. :-( */

So essentially the effect here is that we are disabling scatter-gather
when we segment this since we will have to allocate new buffers to
realign the linear sections.

Looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>