Re: [PATCH net v3 1/1] net/sched: fix pedit partial COW leading to page cache corruption

[David Laight <david.laight.linux@gmail.com>]

On Thu, 28 May 2026 13:53:14 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> On Thu, May 28, 2026 at 6:26 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Thu, May 28, 2026 at 6:03 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:  
> > >
> > > Jamal Hadi Salim <jhs@mojatatu.com> writes:
> > >  
> > > > From: Rajat Gupta <rajat.gupta@oss.qualcomm.com>
> > > >
> > > > tcf_pedit_act() computes the COW range for skb_ensure_writable()
> > > > once before the key loop using tcfp_off_max_hint, but the hint does
> > > > not account for the runtime header offset added by typed keys. This
> > > > can leave part of the write region un-COW'd.
> > > >
> > > > Fix by moving skb_ensure_writable() inside the per-key loop where
> > > > the actual write offset is known, and add overflow checking on the
> > > > offset arithmetic. For negative offsets (e.g. Ethernet header edits
> > > > at ingress), use skb_cow() to COW the headroom instead. Guard
> > > > offset_valid() against INT_MIN, where negation is undefined.  
> > >
> > > So you did tell us not to nitpick, but...
> > >  
> >
> > Actually, an opportunity to nitpick has opened up;-> I have to resend.
> > In my rush to send the patch out i accidentally deleted the "Fixes"
> > while adding names and removing obsolete commit log. Probably missed
> > something else.
> >  
> 
> Sigh. There's another issue that both sashikos pointed out in v2 but i
> wasnt paying attention.
> https://sashiko.dev/#/patchset/20260526155913.1060694-1-jhs%40mojatatu.com
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260526155913.1060694-1-jhs%40mojatatu.com
> 
> I believe this is because we removed
> skb_header_pointer/skb_store_bits() (which ensures you get an error if
> you try to write beyond skb->len)
> 
> Toke/David - Would this be ok? It passes the tests - but i am afraid
> it will require another round of reviews/tests

The length of the bytes being written got lost somewhere.

> 
> static bool offset_valid(struct sk_buff *skb, int offset, int len)
> {
>         if (offset < -(int)skb_headroom(skb))
>                 return false;
> 
>         return offset + len <= (int)skb->len;
> }
> 
> If this is agreable can people test? then i will send the patch

If 'offset' isn't a controlled value you don't want to add 4 to it.
OTOH skb->len is nice and safe.
So the original(ish) comparison:
	return offset <= (int)skb->len - len;
avoids the '+' overflowing.

-- David

> 
> cheers,
> jamal
> 
> 
> > > > 2) Add more optimal boundary checks (Toke & David L.)  
> > >
> > > [..]
> > >  
> > > > -     if  (offset < 0 && -offset > skb_headroom(skb))
> > > > +     if (offset < 0 && offset < -(int)skb_headroom(skb))  
> > >
> > > Seems that bit of the changelog isn't actually accurate.
> > >
> > > However, I don't think this matters, this version is not actually buggy;
> > > so let's just get this merged, and we can code-golf the offset check on
> > > top :)
> > >
> > > I did re-run the tests on this version, and they look fine, so
> > > re-affirming my tags.
> > >
> > > -Toke
> > >