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

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;