Re: [PATCH net-next v1 03/11] xdp: Copy buffers with restricted lifetime

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] xdp: missing xdp_data_meta_unsupported() check in
  xdp_build_skb_from_zc()
- [High] xdp: skb_shared_info overwrite in xdp_build_skb_from_zc() via
  aligned memcpy

--

> diff --git a/net/core/xdp.c b/net/core/xdp.c
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -638,6 +689,9 @@ struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp)
>  	u32 nr_frags = 0;
>  	int metalen;
>  
> +	if (unlikely(xdp_buff_needs_copy(xdp)))
> +		return xdp_build_skb_from_zc(xdp);
> +
>  	if (unlikely(xdp_buff_has_frags(xdp))) {
>  		sinfo = xdp_get_shared_info_from_buff(xdp);
>  		nr_frags = sinfo->nr_frags;

[Severity: High]
This makes xdp_build_skb_from_zc() the XDP_PASS path for restricted
page pool buffers, and its copy can overrun the new head into the
skb_shared_info.

The function asks the system page pool for truesize = xdp->frame_sz.
When that comes back unchanged, napi_build_skb() places
skb_shared_info at the same offset that xdp_data_hard_end() has in the
source. The code then copies LARGEST_ALIGN(len) bytes starting at
offset xdp->data_meta - xdp->data_hard_start.

That offset is often not a multiple of 16, which is normal after
bpf_xdp_adjust_head() by an Ethernet or VLAN header. If data_end is
also close to xdp_data_hard_end(), the rounded-up memcpy writes into
the already initialised skb_shinfo() of the new skb: flags,
nr_frags, the gso fields and frag_list.

The bytes written come from past data_end in the head chunk, which
for UMEM is writable by userspace. Freeing or segmenting the skb
later then walks a bogus nr_frags or frag_list.

With XDP_FLAGS_EXT the head chunk no longer holds the shared info at
all. Does anything still keep data_end below xdp_data_hard_end() for
these buffers? If not, len alone can run past skb->end, since
__skb_put() does not check.

Should the destination size be derived from the copy instead of
xdp->frame_sz? For example: headroom + LARGEST_ALIGN(len) +
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)). Or should this path
copy exactly len bytes?

[Severity: High]
Restricted buffers on XDP_PASS now go through xdp_build_skb_from_zc(),
but that function cannot cope with unsupported metadata. If the driver
called xdp_prepare_buff(..., false), or metadata is otherwise invalid,
xdp->data_meta is xdp->data + 1.

The non-copy path of xdp_build_skb_from_buff() handles this. It
reserves and puts from xdp->data and only sets metadata when
metalen > 0. xdp_copy_buff_to_frame() in this patch also checks
xdp_data_meta_unsupported().

xdp_build_skb_from_zc() does not. It:

- computes len as data_end - data_meta,
- reserves data_meta - data_hard_start,
- copies from data_meta,
- then sees metalen == -1 and skips the __skb_pull().

The resulting skb starts at the second byte of the Ethernet header and
is one byte short, so eth_type_trans() and everything above it parse
shifted data. If data_end == data, len also wraps as a u32.

The XSK callers did not hit this, because the pool sets
data_meta == data. Page pool drivers without metadata support pass
meta_valid == false routinely, though.

Could xdp_build_skb_from_zc() compute the metadata size the way
xdp_copy_buff_to_frame() does? That means treating unsupported
metadata as zero length and copying from xdp->data.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110102039.1002605-1-bjorn%40kernel.org