Re: [PATCH 04/12] xfrm: iptfs: fix runt reassembly panic from short inner tot_len

[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 runt reassembly panic from short inner tot_len

This tightens the runt continuation path in iptfs_reassem_cont() so the
declared inner IP length must cover at least the IP header size before the
in-progress reassembly skb is allocated, replacing the old check against
sizeof(xtfs->ra_runt). A few questions below on how completely this matches
the in-place validation, plus two pre-existing observations in the same
function.

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 2ce15c472cc4d..6920940a35b49 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
> @@ -828,8 +828,8 @@ static u32 iptfs_reassem_cont(struct xfrm_iptfs_data *xtfs, u64 seq,
>  		 * allocate an in progress skb
>  		 */
>  		ipremain = __iptfs_iplen(xtfs->ra_runt);
> -		if (ipremain < sizeof(xtfs->ra_runt)) {
> -			/* length has to be at least runtsize large */
> +		if (ipremain < __iptfs_iphlen(xtfs->ra_runt)) {
> +			/* length has to be at least the IP header size */

[Severity: Medium]
The changelog says this will "Align the runt path with the normal path by
requiring the declared inner length to cover at least the IP header size."
Is that alignment complete for IPv4 with options?

__iptfs_iphlen() never looks at iph->ihl:

net/xfrm/xfrm_iptfs.c:__iptfs_iphlen() {
	if (iph->version == 0x4)
		return sizeof(*iph);
	return sizeof(struct ipv6hdr);
}

The in-place parser uses the declared header length instead:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
	...
	iplen = be16_to_cpu(iph->tot_len);
	iphlen = iph->ihl << 2;
	if (iplen < iphlen || iphlen < sizeof(*iph)) {
		XFRM_INC_STATS(net, LINUX_MIB_XFRMINHDRERROR);
	...
}

So a runt-started inner IPv4 packet declaring ihl < 5, or ihl * 4 greater
than tot_len (for example tot_len 20 with ihl 15), passes the new check,
gets a 20 byte allocation, is treated as completely reassembled and is
handed to iptfs_complete_inner_skb() and xfrm_input(), while byte-identical
input arriving wholly inside one outer payload is rejected with
LINUX_MIB_XFRMINHDRERROR. Should the runt path also test iph->ihl so the
two parsers agree on both acceptance and MIB accounting?

As far as I can tell the residual effect is bounded, since ip_rcv_core()
drops such a packet:

net/ipv4/ip_input.c:ip_rcv_core() {
	if (iph->ihl < 5 || iph->version != 4)
		goto inhdr_error;
	...
	if (!pskb_may_pull(skb, iph->ihl*4))
		goto inhdr_error;
}

The panic class described in the changelog (tot_len in [6, 19] making
"ipremain -= copylen" underflow) does look closed by this change for the
runt path.

Also, the kernel-doc on __iptfs_iphlen() says it returns "the IP header
size based on the IP version", and the new comment here says "length has to
be at least the IP header size". For IPv4 with options that is the minimum
header size rather than the header size. Would it be worth rewording either
the comment or the helper's kernel-doc?

>  			XFRM_INC_STATS(xs_net(xtfs->x),
>  				       LINUX_MIB_XFRMINIPTFSERROR);
>  			goto abandon;

Two further observations in this same function, both outside the hunk.

[Severity: High]
This isn't a bug introduced by this patch, but can the copy fallback later
in iptfs_reassem_cont() still reach skb_over_panic() on the normal
(non-runt) path?

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
	} else {
		if (skb_linearize(newskb)) {
			XFRM_INC_STATS(xs_net(xtfs->x),
				       LINUX_MIB_XFRMINBUFFERERROR);
			goto abandon;
		}

		/* copy fragment data into newskb */
		if (skb_copy_seq_read(st, data, skb_put(newskb, copylen),
				      copylen)) {
	...
}

There is no tailroom check before skb_put(newskb, copylen), and copylen is
min(min(blkoff, remaining), ipremain), so it is bounded only by the outer
payload length and the declared inner IP length.

When the in-progress skb came from the frag-sharing path, its linear head
is only iphlen plus XFRM_IPTFS_MIN_L2HEADROOM:

net/xfrm/xfrm_iptfs.c:iptfs_pskb_add_frags() {
	skb = iptfs_alloc_skb(tpl, copy_len, false);
	...
}

reached from __input_process_payload():

			    !(skb = iptfs_pskb_add_frags(first_skb, fragwalk,
							 data, capturelen,
							 skbseq, iphlen))) {

and skb_linearize() only adds about 128 bytes of tailroom:

net/core/skbuff.c:__pskb_pull_tail() {
	int i, k, eat = (skb->tail + delta) - skb->end;
	...
	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))
	...
}

A continuation outer packet that is a plain kmalloc'd linear skb fails the
sharing precondition (head_frag is 0 and skb->len != skb->data_len), so it
lands in this fallback. With a roughly 1420 byte reassembly the head ends
up with a couple of hundred bytes of tailroom, and a continuation carrying
several hundred bytes more makes skb->tail exceed skb->end, which
skb_put() turns into skb_over_panic() and BUG(). Would guaranteeing the
tailroom (pskb_expand_head() or a skb_tailroom() test against ipremain)
before the skb_put() be appropriate here? This path still appears to be
present at the tip of the series.

[Severity: Low]
This is a pre-existing asymmetry rather than something this patch changes,
but should the runt path also arm the drop timer when it publishes
xtfs->ra_newskb?

net/xfrm/xfrm_iptfs.c:iptfs_reassem_cont() {
		newskb = iptfs_alloc_skb(skb, ipremain, false);
		if (!newskb) {
			XFRM_INC_STATS(xs_net(xtfs->x), LINUX_MIB_XFRMINERROR);
			goto abandon;
		}
		xtfs->ra_newskb = newskb;
	...
}

The reassembly-start site in __input_process_payload() does arm it:

			xtfs->ra_newskb = skb;
			xtfs->ra_wantseq = seq + 1;
			if (!hrtimer_is_queued(&xtfs->drop_timer)) {
				hrtimer_start(&xtfs->drop_timer,
					      xtfs->drop_time_ns,
					      IPTFS_HRTIMER_MODE);
			}

So a reassembly started from the runt path has no expiry, and if the peer
never sends the next sequence the skb (sized by the declared inner length,
up to about 64KB) stays pinned on the SA until another packet arrives or
the state goes away. The memory is reclaimed at teardown:

net/xfrm/xfrm_iptfs.c:iptfs_destroy_state() {
	hrtimer_cancel(&xtfs->drop_timer);

	if (xtfs->ra_newskb)
		kfree_skb(xtfs->ra_newskb);
	...
}

so this looks like retention past the intended drop time rather than a
leak. The publication itself is serialized, since iptfs_input_ordered()
holds xtfs->drop_lock across the iptfs_reassem_cont() call.