netdev - Re: [net,1/2] net: add skb_header_pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20260128025556.4169217-1-kuba@kernel.org>
Date: Tue, 27 Jan 2026 18:55:56 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: edumazet@...gle.com
Cc: Jakub Kicinski <kuba@...nel.org>,
	horms@...nel.org,
	jhs@...atatu.com,
	davem@...emloft.net,
	jiri@...nulli.us,
	km.kim1503@...il.com,
	pabeni@...hat.com,
	xiyou.wangcong@...il.com,
	eric.dumazet@...il.com,
	netdev@...r.kernel.org
Subject: Re: [net,1/2] net: add skb_header_pointer_careful() helper

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