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 <[email protected]> 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 <[email protected]>
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
