> 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?
> skb_shinfo(skb)->frag_list = NULL;
>
> while (list_skb) {
> @@ -4109,6 +4119,9 @@ struct sk_buff *skb_segment_list(struct sk_buff
> *skb,
> __skb_linearize(skb))
> goto err_linearize;
>
> + if (err_len)
> + goto err_linearize;
> +
> skb_get(skb);
>
> return skb;
>
> > >
> > > > Back to the original report: the issue should already have been
> > fixed
> > > > by commit 876e8ca83667 ("net: fix NULL pointer in
> > skb_segment_list").
> > > > But that commit is in the kernel for which you report the error.
> > > >
> > > > Turns out that the crash is not in skb_segment_list, but later in
> > > > __udpv4_gso_segment_list_csum. Which unconditionally dereferences
> > > > udp_hdr(seg).
> > > >
> > > > The above fix also mentions skb pull as the culprit, but does not
> > > > include a BPF program. If this can be reached in other ways, then
> > we
> > > > do need a stronger test in skb_segment_list, as you propose.
> > > >
> > > > I don't want to narrowly check whether udp_hdr is safe.
> > Essentially,
> > > > an SKB_GSO_FRAGLIST skb layout cannot be trusted at all if even
> > one
> > > > byte would get pulled.
