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

[Simon Glass]

Hi Mateusz,

On Wed, 3 Jun 2026 at 17:23, Mateusz Furdyna <mateusz.furdyna at nokia.com> wrote:
>
> 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?

That seems fine to me.

>
> 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).

Yes that's good.

>
> >
> > 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