[PATCH net v2] skbuff: Account for tail adjustment during pull operations

[PATCH net v2] skbuff: Account for tail adjustment during pull operations

[Subash Abhinov Kasiviswanathan]

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@quicinc.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.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. :-( */
-- 
2.7.4

Re: [PATCH net v2] skbuff: Account for tail adjustment during pull operations

[Alexander H Duyck]

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@quicinc.com>
> Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@quicinc.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@fb.com>

Re: [PATCH net v2] skbuff: Account for tail adjustment during pull operations

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 14 Dec 2022 23:11:58 -0700 you 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
> 
> [...]

Here is the summary with links:
  - [net,v2] skbuff: Account for tail adjustment during pull operations
    https://git.kernel.org/netdev/net/c/2d7afdcbc9d3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html