[PATCH net] inet: frags: fix a possible frag_mem_limit drift

[PATCH net] inet: frags: fix a possible frag_mem_limit drift

[Eric Dumazet]

During IP fragment reassembly, when an incoming fragment completes
the reassembly but is not the first fragment
(the head != skb branch in inet_frag_reasm_prepare),
the code creates a clone fp of the incoming fragment skb and
swaps it into the rbtree.

The original fragment (skb) had its truesize added to the fragmentation
memory limit fqdir->mem during ip_frag_queue.

However, the clone fp can have a different truesize.

When the reassembly finishes, inet_frag_reasm_finish traverses the tree
and subtracts the different fp->truesize from fqdir->mem.

We probably never have seen this issue in the wild because
most NIC are cooking packets with a constant skb->truesize
regardless of the fragment size (Assuming MTU <= 1500).

However this could be exploited for NIC implementing copybreak
(copying small payload in skb->head), or MTU >= 4096
and NIC using 1 to 3 4K pages for jumbo frames.

Fixes: 1706d58763c3 ("[IPV4]: Make ip_defrag return the same packet")
Reported-by: Damiano Melotti <melotti@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Cc: Florian Westphal <fw@strlen.de>
Cc: Jesper Dangaard Brouer <hawk@kernel.org>
Cc: Peter Oskolkov <posk@google.com>
---
 net/ipv4/inet_fragment.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 393770920abd794a47a933f2694afe3e2d08d25a..3222cc2ac235e9628da88db6637498755749b07d 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -539,6 +539,9 @@ void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
 		FRAG_CB(skb)->next_frag = FRAG_CB(head)->next_frag;
 		rb_replace_node(&head->rbnode, &skb->rbnode,
 				&q->rb_fragments);
+		delta = skb->truesize - head->truesize;
+		if (delta)
+			add_frag_mem_limit(q->fqdir, delta);
 		consume_skb(head);
 		head = skb;
 	}
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH net] inet: frags: fix a possible frag_mem_limit drift

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
inet: frags: fix a possible frag_mem_limit drift

This patch attempts to compensate for a frag_mem_limit accounting drift
in inet_frag_reasm_prepare() when an incoming fragment that completes
reassembly is not the first fragment, by adding the difference between
skb->truesize and head->truesize to fqdir->mem before consuming head.

> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 393770920abd..3222cc2ac235 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -539,6 +539,9 @@ void *inet_frag_reasm_prepare(struct inet_frag_queue *q, struct sk_buff *skb,
>  		FRAG_CB(skb)->next_frag = FRAG_CB(head)->next_frag;
>  		rb_replace_node(&head->rbnode, &skb->rbnode,
>  				&q->rb_fragments);
> +		delta = skb->truesize - head->truesize;
> +		if (delta)
> +			add_frag_mem_limit(q->fqdir, delta);

[High]
Can the new delta computation ever evaluate to a non-zero value at this
point?

Looking a few lines above in the same function, skb_morph(skb, head) is
invoked before this hunk:

	skb_morph(skb, head);
	FRAG_CB(skb)->next_frag = FRAG_CB(head)->next_frag;
	rb_replace_node(&head->rbnode, &skb->rbnode,
			&q->rb_fragments);
	delta = skb->truesize - head->truesize;

In net/core/skbuff.c, skb_morph() is implemented as:

	struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
	{
		skb_release_all(dst, SKB_CONSUMED);
		return __skb_clone(dst, src);
	}

and __skb_clone() copies truesize from src to dst via the C(x) macro:

	#define C(x) n->x = skb->x
	...
	C(truesize);

So by the time the new code runs, skb->truesize has already been
overwritten with head->truesize, and the subtraction is identically
zero. The if (delta) branch never fires and add_frag_mem_limit() is
never called.

If the drift described in the changelog is real (the original
skb->truesize accounted via ip_frag_queue() differs from the clone fp's
truesize that ends up being subtracted later), would the compensation
need to be computed before skb_morph() overwrites skb->truesize, or
expressed against fp->truesize rather than the post-morph skb->truesize?

>  		consume_skb(head);
>  		head = skb;
>  	}
-- 
pw-bot: cr