Re: [PATCH bpf-next 36/38] bpf: Follow the head if an skb is written from a slice

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf: duplicate bpf_try_make_writable() call in __bpf_skb_store_bytes()
--

commit cd64e2e5d6a96038e5e37496663c8035d9bcd26c
Author: Josef Bacik <josef@toxicpanda.com>

bpf: Follow the head if an skb is written from a slice

This commit introduces bpf_try_make_writable_from() and updates
__bpf_skb_store_bytes() and __bpf_skb_meta_store_bytes() to adjust the source
pointer when making an skb writable reallocates its head.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index bf01152d0cf9a..54e9084b9263a 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1752,9 +1752,32 @@ static const struct bpf_func_proto bpf_skb_store_bytes_proto = {
>  	.arg5_type	= ARG_ANYTHING,
>  };
>  
> +/*
> + * The source of a write through a dynptr may be a slice of the same skb.
> + * Making the skb writable can move its head: follow the move.
> + */
> +static int bpf_try_make_writable_from(struct sk_buff *skb, u32 write_len,
> +				      const void **from)
> +{
> +	const void *head = skb->head;
> +	bool in_head = *from >= head &&
> +		       *from < (const void *)skb_end_pointer(skb);
> +	int err;
> +
> +	err = bpf_try_make_writable(skb, write_len);
> +	if (!err && in_head)
> +		*from = skb->head + (*from - head);
> +	return err;
> +}
> +
>  int __bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from,
>  			  u32 len, u64 flags)
>  {
> +	if (unlikely(offset > INT_MAX))
> +		return -EFAULT;
> +	if (unlikely(bpf_try_make_writable_from(skb, offset + len, &from)))
> +		return -EFAULT;
> +
>  	return ____bpf_skb_store_bytes(skb, offset, from, len, flags);
>  }

[Severity: Medium]
Does calling ____bpf_skb_store_bytes() after bpf_try_make_writable_from() end
up calling bpf_try_make_writable() a second time without updating from?

Looking at ____bpf_skb_store_bytes() in net/core/filter.c:

    BPF_CALL_5(bpf_skb_store_bytes, struct sk_buff *, skb, u32, offset,
               const void *, from, u32, len, u64, flags)
    {
        void *ptr;

        if (unlikely(flags & ~(BPF_F_RECOMPUTE_CSUM | BPF_F_INVALIDATE_HASH)))
            return -EINVAL;
        if (unlikely(offset > INT_MAX))
            return -EFAULT;
        if (unlikely(bpf_try_make_writable(skb, offset + len)))
            return -EFAULT;

        ptr = skb->data + offset;
        if (flags & BPF_F_RECOMPUTE_CSUM)
            __skb_postpull_rcsum(skb, ptr, len, offset);

        memcpy(ptr, from, len);

When a BPF program calls bpf_dynptr_write() or bpf_dynptr_copy() on a
BPF_DYNPTR_TYPE_SKB dynptr using a slice of the same skb as the source,
__bpf_skb_store_bytes() calls bpf_try_make_writable_from() and then calls
____bpf_skb_store_bytes().

Inside ____bpf_skb_store_bytes(), the second bpf_try_make_writable() call
follows the chain:

    bpf_try_make_writable()
      __bpf_try_make_writable()
        skb_ensure_writable()
          pskb_may_pull()
            pskb_may_pull_reason()
              skb_might_realloc()
                pskb_expand_head()

When CONFIG_FAIL_SKB_REALLOC triggers, skb_might_realloc() reallocates and
frees skb->head during that second call without updating from, leaving from
pointing to the freed head before copying into ptr.

Also, because __bpf_skb_store_bytes() calls bpf_try_make_writable_from()
before ____bpf_skb_store_bytes() checks flags, invalid flags will only be
rejected after the skb has already been uncloned or pulled (and will return
-EFAULT instead of -EINVAL if skb_ensure_writable() fails, unlike
__bpf_skb_meta_store_bytes() which checks flags first).

Note that passing packet slices directly to the bpf_skb_store_bytes() or
bpf_lwt_seg6_store_bytes() helpers is prevented later in the series by commit
"bpf: Refuse packet slices for packet-changing helpers", so could
bpf_try_make_writable_from() be used directly inside ____bpf_skb_store_bytes()
instead of duplicating the checks in __bpf_skb_store_bytes()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017012058.355787-1-alexei.starovoitov@gmail.com?part=36