[PATCH] net: clear IP defragmentation state after returning a complete packet

[Mateusz Furdyna]

Hi Simon,
Thank you for a review!

(I apologize in advance for any issues caused by my mail client's 
potentially still incorrect configuration)

On 03/06/2026 19:04, Simon Glass wrote:
> Hi Mateusz,
> 
> On 2026-06-01T22:48:21, Mateusz Furdyna <mateusz.furdyna at nokia.com> wrote:
>> net: clear IP defragmentation state after returning a complete packet
>>
>> During the IP defragmentation process, after the reassembly is finished
>> with the last packet arriving with MF=0, the reassembly state wrt.
>> static counters is not cleared. In case this last arriving packet with
>> MF=0 gets duplicated, payload bytes are mistakingly treated as hole data.
>>
>> A malicious actor who can deliver fragmented IP traffic to a U-Boot
>> instance with CONFIG_IP_DEFRAG=y can corrupt memory via out-of-bound
>> writes and redirect control flow into attacker-supplied payload bytes
>> that already sit in pkt_buff[].
>>
>> Publicly available AI models are able to generate a reproducer based
>> on the provided information.
>>
>> Fix: once the assembled packet has been handed back to the caller, mark
>> the reassembly state empty so that any further fragment (duplicate,
>> replay, or a brand-new datagram that happens to reuse the ip_id) goes
>> through the normal re-init path and rebuilds a clean hole list instead
>> of dereferencing payload bytes as struct hole.
>> [...]
>>
>> net/net.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> with nits below
> 
>> diff --git a/net/net.c b/net/net.c
>> @@ -1103,6 +1103,14 @@ static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int *lenp)
>>
>>        *lenp = total_len + IP_HDR_SIZE;
>>        localip->ip_len = htons(*lenp);
>> +
>> +     /* Mark the reassembly state empty so that any further
>> +      * fragment goes through the normal re-init path and
>> +      * rebuilds a clean hole list
>> +      */
> 
> Please use the standard U-Boot multi-line comment style with /* on its
> own line, to match the rest of this file (see lines 962 and 1016):
> 
>     /*
>      * Mark the reassembly state empty so that any further
>      * fragment goes through the normal re-init path and
>      * rebuilds a clean hole list.
>      */
> 

Ack, will do.

>> diff --git a/net/net.c b/
>> @@ -1103,6 +1103,14 @@ static struct ip_udp_hdr *__net_defragment(struct ip_udp_hdr *ip, int *lenp)
>> +     total_len = 0;
>> +     first_hole = 0;
>> +
>>        return localip;
>>   }
> 
> Only total_len = 0 is actually needed, since the re-init path at line
> 1007 already resets first_hole (and payload[0]) whenever !total_len is
> true. Still, this doesn't hurt.

Ack, will keep the first_hole = 0 for the sake of "defensive programming".

> 
> Any chance of adding a test? Probably this is a little tricky...

Hmm. __net_defragment() is called from net_defragment() when 
CONFIG_IP_DEFRAG=y, which in turn is called from 
net_process_received_packet().

net_process_received_packet() is finally declared in
include/net-common.h and reachable from outside the compilation unit of 
net.c.

If this isn't too much of a stretch, I'd propose creating a new file in 
test/dm/net_defrag.c and reuse the existing test infrastructure here. 
The stretch is this placement in `dm`. What could be a better choice?

The test would pass a 2-fragment, pretty short datagram:
- frag A = MF=1, 8 bytes of data
- frag B = MF=0, 8 bytes of data

After completion the unfixed code leaves total_len=16, first_hole=1, and 
payload[1] now holds frag B's data bytes. On a duplicate of frag B, the 
function does h = payload + first_hole and reinterprets those data bytes 
as a struct hole.

Now, in order to actually have a deterministic and reliably failing test 
for the unfixed code, we could:
- first, use net_set_udp_handler() to count incoming datagrams
- craft B's 8 data bytes to look like this "benign" hole
	last_byte = 16, next_hole = 0, prev_hole = 0
- abuse the delivery condition (net.c lines 1054-1058):

	if ((h >= thisfrag) && (h->last_byte <= start + len)) {
		/* complete overlap with hole: remove hole */
		if (!h->prev_hole && !h->next_hole) {
			/* last remaining hole */
			done = 1;

Duplicated packet B would trigger duplicated delivery condition, call 
handler registered using net_set_udp_handler() and increase the counter 
in case of unfixed version; fixed version would count only a single UDP 
packet in the test scenario. No segfaults.

Sounds like a plan? Let me know if test/dm is an acceptable place for 
this and I will write it in a separate commit (same patchset).

> 
> BTW, 'mistakingly' in the commit message should be 'mistakenly'.

Thank you, honestly, that's a way to finally remember the correct 
spelling :D. Will fix the commit message.

> 
> Regards,
> Simon