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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
xfrm: iptfs: fix stack OOB read in iptfs_skb_reset_frag_walk()

This adds an up-front range check to iptfs_skb_can_add_frags() so that
iptfs_skb_reset_frag_walk() is never entered with an offset at or beyond
the end of the walk's fragment data, where its unbounded advance loop
would read past walk->frags[]. The guard mirrors the one already present
in iptfs_skb_add_frags().

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 597aedeac26eb..2ce15c472cc4d 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -416,6 +416,14 @@ 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;
>  
> +	/* 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;
> +

[Severity: Medium]

Should this return true (or be conditioned on len) instead of false?

In the iptfs_reassem_cont() path the walk is prepared and then queried
with the same offset:

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
	...
	fraglen = min(blkoff, remaining);
	copylen = min(fraglen, ipremain);

	if (!skb_has_frag_list(skb) && !skb_has_frag_list(newskb) &&
	    (skb->head_frag || skb->len == skb->data_len) &&
	    skb->pp_recycle == newskb->pp_recycle) {
		fragwalk = &_fragwalk;
		iptfs_skb_prepare_frag_walk(skb, data, fragwalk);
	}

	if (fragwalk &&
	    iptfs_skb_can_add_frags(newskb, fragwalk, data, copylen)) {
	...
}

Since offset == walk->initial_offset == data here, the new clause reduces
to walk->total == 0, and iptfs_skb_prepare_frag_walk() sums exactly
skb->len - data, so it only fires when data == skb->len, i.e. when
remaining == 0 and therefore copylen == 0 as well.

For that case the guard now sends control into the else branch:

	} else {
		if (skb_linearize(newskb)) {
			...
		}

		/* copy fragment data into newskb */
		if (skb_copy_seq_read(st, data, skb_put(newskb, copylen),
				      copylen)) {
			XFRM_INC_STATS(xs_net(xtfs->x),
				       LINUX_MIB_XFRMINBUFFERERROR);
			goto abandon;
		}
	}

with copylen == 0 and data == skb->len.  skb_copy_seq_read() calls
skb_seq_read() before it looks at len:

net/core/skbuff.c:skb_copy_seq_read() {
	for (;;) {
		sqlen = skb_seq_read(offset, &data, st);
		if (sqlen == 0)
			return -EINVAL;
		if (sqlen >= len) {
	...
}

and skb_seq_read() returns 0 once abs_offset >= st->upper_offset, so the
zero-length copy returns -EINVAL and iptfs_reassem_cont() takes the
abandon label, where iptfs_reassem_abort() frees xtfs->ra_newskb and
destroys the in-progress inner packet reassembly.

Before this patch the same input took the sharing branch: can_add_frags()
fell through its "while (len && fragi < walk->nr_frags)" loop with len == 0
and returned true, iptfs_skb_add_frags() returned immediately via its own
!walk->nr_frags check, and reassembly continued via
"if (copylen < ipremain) xtfs->ra_wantseq++;".

Two inputs from a peer on an IPTFS SA appear to reach this: an AGGFRAG
payload carrying only the basic/CC header with block_offset >= ipremain
while a reassembly is in progress, and the "newskb->len < iphlen" block
copying exactly iphremain == remaining bytes so that "data += copylen"
leaves data == skb->len.

Would a guard on len, for example returning true when len == 0 and only
rejecting when there is data left to add, close the stack out-of-bounds
read without changing the reassembly outcome?

The other caller, iptfs_pskb_add_frags() in __input_process_payload(), is
only reached when capturelen > iphlen and remaining >= iphlen, so it does
not appear to be able to hit the new guard.

For reference, this behaviour is unchanged through the rest of the series:
the only later change to net/xfrm/xfrm_iptfs.c is the ra_runt iplen check
in "xfrm: iptfs: fix runt reassembly panic from short inner tot_len", and
iptfs_skb_can_add_frags() is the same at the end of the series.