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@xxxxxxxxxxx>
Signed-off-by: Subash Abhinov Kasiviswanathan <quic_subashab@xxxxxxxxxxx>
---
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);
}
