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

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 <redacted>
Signed-off-by: Subash Abhinov Kasiviswanathan <redacted>
---
 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;
+	}
+
 	return skb_ensure_writable(skb, write_len);
 }
 

-- 
2.7.4

On 12/13/22 5:39 AM, Subash Abhinov Kasiviswanathan wrote:

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

On 12/13/2022 3:42 PM, Daniel Borkmann wrote:

On 12/13/22 5:39 AM, Subash Abhinov Kasiviswanathan wrote:

quoted

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. 
:-( */

quoted

      return skb_ensure_writable(skb, write_len);
  }

On 12/14/22 7:32 AM, Subash Abhinov Kasiviswanathan (KS) wrote:

On 12/13/2022 3:42 PM, Daniel Borkmann wrote:

quoted

On 12/13/22 5:39 AM, Subash Abhinov Kasiviswanathan wrote:

quoted

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.