On 12/13/2022 3:42 PM, Daniel Borkmann wrote:
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 -
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.
:-( */
return skb_ensure_writable(skb, write_len);
}
