Re: [PATCH 01/12] xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

[Steffen Klassert <steffen.klassert@secunet.com>]

On Tue, Sep 15, 2026 at 08:31:54AM -0000, Roshan Kumar wrote:
> Hi Steffen,
> 
> I had a look at the review and it is right that the guard changes the
> len == 0 outcome, with one nuance worth splitting out.
> 
> The sharing branch needs a prepared frag walk, so the guard can only
> change behavior for skbs that are frag walk eligible (head_frag set, or
> all data in frags). For those, before the change
> iptfs_skb_can_add_frags() fell through the "while (len && fragi <
> walk->nr_frags)" loop and returned true, iptfs_skb_add_frags()
> returned immediately on its own " !walk->nr_frags || offset out of
> range" check, and reassembly continued with ra_wantseq++. With the
> guard the same input returns false, takes the copy branch, and
> skb_copy_seq_read(..., 0) returns EINVAL, so the in progress
> reassembly is dropped.
> 
> For linear skbs the frag walk stays NULL and this corner dropped
> reassembly before the change too: the copy branch runs either way and
> skb_seq_read at the end of the buffer fails the same way. I reproduced
> that part live on v7.3-rc3 today: a partial inner packet followed by
> an AGGFRAG basic header only block with block_offset 0xffff kills the
> in progress reassembly with and without the fix, so that part already
> existed rather than being something the guard introduces.
> 
> The review's suggestion closes the gap for the frag walk case: return
> true when len == 0, before the offset check. The dangerous walk in
> iptfs_skb_reset_frag_walk() is skipped entirely for len == 0, and
> iptfs_skb_add_frags() keeps its own bounds check for len > 0, so the
> out of bounds read cannot come back this way. The reassembly outcome
> stays identical to before the fix for head frag skbs, so there is no
> efficiency cost either.
> 
> Something like this on top of the patch:
> 
> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ static bool iptfs_skb_can_add_frags(const struct sk_buff *skb,
>         if (skb_has_frag_list(skb) || skb->pp_recycle != walk->pp_recycle)
>                 return false;
> 
> +       /* len == 0: nothing to add, proceed as before the fix. */
> +       if (!len)
> +               return true;

That's ok with me. But drop the comment above, this does not
give any usefull information.

> +
>         /* Reject an @offset that is at or beyond the end of the walk's data
>          * before calling iptfs_skb_reset_frag_walk(), whose fragment-advance
>          * loop is otherwise unbounded and would index past walk->frags[].
>          * This mirrors the guard already present in iptfs_skb_add_frags().
>          */
>         if (!walk->nr_frags || offset >= walk->total + walk->initial_offset)
>                 return false;
> 
> The len == 0 drop for linear skbs existed before this change; I am
> happy to look at that separately once this series lands.

Thanks!