Thread (48 messages) 48 messages, 7 authors, 12d ago

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

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: 2026-09-14 09:19:31

Ccing Author of the patch and author of IPTFS.

On Tue, Sep 08, 2026 at 03:48:55PM -0700, Jakub Kicinski wrote:
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.
quoted
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.
I think this is OK as is. The panic is fixed and it did not add a
regression.
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?
quoted
 			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?
This is not a regression introduced by this patch. Can be fixed as
followup.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help