Re: [net,1/2] net: add skb_header_pointer_careful() helper

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: add skb_header_pointer_careful() helper

This patch adds a new helper to validate negative offsets before calling
skb_header_pointer().  The commit message states that negative offsets
are supported "as long as the zone starts between skb->head and
skb->data."

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 86737076101d..72679aa7af58 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4301,6 +4301,18 @@ skb_header_pointer(const struct sk_buff *skb, int offset, int len, void *buffer)
>  				    skb_headlen(skb), buffer);
>  }
>
> +/* Variant of skb_header_pointer() where @offset is user-controlled
> + * and potentially negative.
> + */
> +static inline void * __must_check
> +skb_header_pointer_careful(const struct sk_buff *skb, int offset,
> +			   int len, void *buffer)
> +{
> +	if (unlikely(offset < 0 && -offset > skb_headlen(skb)))
> +		return NULL;

Should this check use skb_headroom() instead of skb_headlen()?

The commit message says negative offsets should be valid when the zone
is "between skb->head and skb->data" - that describes the headroom, not
the headlen.  For a negative offset, skb_header_pointer() returns
skb->data + offset, which points into the headroom area.

skb_headlen() returns the linear data length after skb->data (skb->len
minus skb->data_len), while skb_headroom() returns the space before
skb->data (skb->data minus skb->head).

These values are quite different in practice - headroom is typically
small (2-128 bytes), while headlen can be up to MTU size (~1500 bytes).
With the current check, an offset like -100 would pass validation when
skb_headlen is 1500, but could read before skb->head if skb_headroom is
only 2 bytes.

The existing offset_valid() function in net/sched/act_pedit.c uses
skb_headroom() for this same type of check:

    if  (offset < 0 && -offset > skb_headroom(skb))
        return false;

> +	return skb_header_pointer(skb, offset, len, buffer);
> +}
-- 
pw-bot: cr