Lena Wang (王娜) wrote: > On Tue, 2024-04-23 at 14:35 -0400, Willem de Bruijn wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > > Hi Willem, > > > As the discussion, is it OK for the patch below? > > > > Thanks for iterating on this. > > > > I would like the opinion also of the fraglist and UDP GRO experts. > > > > Yes, I think both > > > > - protecting skb_segment_list against clearly illegal fraglist > > packets, and > > - blocking BPF from constructing such packets > > > > are worthwhile stable fixes. I believe they should be two separate > > patches. Both probably with the same Fixes tag: 3a1296a38d0c > > ("net: Support GRO/GSO fraglist chaining"). > > > > > diff --git a/net/core/filter.c b/net/core/filter.c > > > index 3a6110ea4009..abc6029c8eef 100644 > > > --- a/net/core/filter.c > > > +++ b/net/core/filter.c > > > @@ -1655,6 +1655,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, > > > bpf_sp); > > > static inline int __bpf_try_make_writable(struct sk_buff *skb, > > > unsigned int write_len) > > > { > > > + if (skb_is_gso(skb) && (skb_shinfo(skb)->gso_type & > > > + SKB_GSO_FRAGLIST) && (write_len > > > > skb_headlen(skb))) { > > > + return -ENOMEM; > > > + } > > > + > > > > Indentation looks off, but I agree with the logic. > > > > if (skb_is_gso(skb) && > > (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) && > > (write_len > skb_headlen(skb))) > > > > > return skb_ensure_writable(skb, write_len); > > > } > > > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > > index 73b1e0e53534..2e90534c1a1e 100644 > > > --- a/net/core/skbuff.c > > > +++ b/net/core/skbuff.c > > > @@ -4036,9 +4036,11 @@ struct sk_buff *skb_segment_list(struct > > sk_buff > > > *skb, > > > unsigned int tnl_hlen = skb_tnl_header_len(skb); > > > unsigned int delta_truesize = 0; > > > unsigned int delta_len = 0; > > > + unsigned int mss = skb_shinfo(skb)->gso_size; > > > struct sk_buff *tail = NULL; > > > struct sk_buff *nskb, *tmp; > > > int len_diff, err; > > > + bool err_len = false; > > > > > > skb_push(skb, -skb_network_offset(skb) + offset); > > > > > > @@ -4047,6 +4049,14 @@ struct sk_buff *skb_segment_list(struct > > sk_buff > > > *skb, > > > if (err) > > > goto err_linearize; > > > > > > + if (mss != GSO_BY_FRAGS && mss != skb_headlen(skb)) { > > > + if (!list_skb) { > > > + goto err_linearize; > > > > The label no longer truly covers the meaning. > > > > But that is already true since the above (second) jump was added in > > commit c329b261afe7 ("net: prevent skb corruption on frag list > > segmentation"). > > > > Neither needs the kfree_skb_list, as skb->next is not assigned to > > until the loop. Can just return ERR_PTR(-EFAULT)? > > > > > + } else { > > > + err_len = true; > > > + } > > > + } > > > + > > > > Why the branch? Might as well always fail immediately? > > > Hi Willem, > Thanks for your guidance. > You are right. There is no need for another branch as fraglist > could be freeed in kfree_skb. > Could I git send mail wo patches as below? > > From 933237400c0e2fa997470b70ff0e407996fa239c Mon Sep 17 00:00:00 2001 > From: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx> > Date: Wed, 24 Apr 2024 13:42:35 +0800 > Subject: [PATCH net] net: prevent BPF pull GROed skb's fraglist > > A GROed skb with fraglist can't be pulled data Please use the specific label: SKB_GSO_FRAGLIST skb > from its fraglist as it may result a invalid > segmentation or kernel exception. > > For such structured skb we limit the BPF pull > data length smaller than skb_headlen() and return > error if exceeding. > > Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.") > Signed-off-by: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx> > Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx> > --- > net/core/filter.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 8adf95765cdd..8ed4d5d87167 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, > bpf_sp); > static inline int __bpf_try_make_writable(struct sk_buff *skb, > unsigned int write_len) > { > + if (skb_is_gso(skb) && > + (skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST) && > + write_len > skb_headlen(skb)) { > + return -ENOMEM; > + } > return skb_ensure_writable(skb, write_len); > } > > -- > 2.18.0 > > > From 2d0729b20cf810ba1b31e046952c1cd78f295ca3 Mon Sep 17 00:00:00 2001 > From: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx> > Date: Wed, 24 Apr 2024 14:43:45 +0800 > Subject: [PATCH net] net: drop GROed skb pulled from fraglist > > A GROed skb with fraglist maybe pulled by BPF > or other ways. It can't be trusted at all even > if one byte is pulled and should be dropped > on segmentation. This paraphrases my comment. It is better to spell it out: An SKB_GSO_FRAGLIST skb without GSO_BY_FRAGS is expected to have all segments except the last to be gso_size long. If this does not hold, the skb has been modified and the fraglist gso integrity is lost. Drop the packet, as it cannot be segmented correctly by skb_segment_list. The skb could be salvaged, though, right? By linearizing, dropping the SKB_GSO_FRAGLIST bit and entering the normal skb_segment path rather than the skb_segment_list path. That choice is currently made in the protocol caller, __udp_gso_segment. It's not trivial to add such a backup path here. So let's add this backstop against kernel crashes. > > If the gso_size does not match skb_headlen(), > it means to be pulled part of or the entire > fraglsit. It has been messed with and we return fraglist > error to free this skb. > > Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.") > Signed-off-by: Shiming Cheng <shiming.cheng@xxxxxxxxxxxx> > Signed-off-by: Lena Wang <lena.wang@xxxxxxxxxxxx> > --- > net/core/skbuff.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index b99127712e67..750fbb51b99f 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -4493,6 +4493,7 @@ struct sk_buff *skb_segment_list(struct sk_buff > *skb, > unsigned int tnl_hlen = skb_tnl_header_len(skb); > unsigned int delta_truesize = 0; > unsigned int delta_len = 0; > + unsigned int mss = skb_shinfo(skb)->gso_size; Reverse christmas tree > struct sk_buff *tail = NULL; > struct sk_buff *nskb, *tmp; > int len_diff, err; > @@ -4504,6 +4505,9 @@ struct sk_buff *skb_segment_list(struct sk_buff > *skb, > if (err) > goto err_linearize; > > + if (mss != GSO_BY_FRAGS && mss != skb_headlen(skb)) > + return ERR_PTR(-EFAULT); > + Do this precondition integrity check before the skb_unclone path? > skb_shinfo(skb)->frag_list = NULL; > > while (list_skb) { > -- > 2.18.0