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: <CANn89i+W13kkNwpW6YPeZswf_WJR4g_TMGS_27cU2fHS8KKG-w@mail.gmail.com>
Date: Wed, 28 Jan 2026 15:01:13 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: 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

On Wed, Jan 28, 2026 at 3:56 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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);
> > +}

Yes, of course, silly error on my side.