From 56b29f8bcfaefe2974dca67bde16cc7c391feaeb Mon Sep 17 00:00:00 2001 From: Simon Goldschmidt Date: Tue, 3 Jun 2025 21:04:39 +0200 Subject: [PATCH] ip4_frag/ip6_frag: fix potential NULL-pointer access on memory errors --- CHANGELOG | 5 ++++ src/core/ipv4/ip4_frag.c | 28 ++++++++++--------- src/core/ipv6/ip6_frag.c | 58 +++++++++++++++++++++------------------- 3 files changed, 50 insertions(+), 41 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 90a5834c..355175b6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -6,6 +6,11 @@ HISTORY * [Enter new changes just after this line - do not remove this line] + ++ Bugfixes: + + 2025-06-03: Simon Goldschmidt + * ip4_frag/ip6_frag: fix potential NULL-pointer access on memory errors + (STABLE-2.2.1): ++ New features: diff --git a/src/core/ipv4/ip4_frag.c b/src/core/ipv4/ip4_frag.c index 53031447..2652487b 100644 --- a/src/core/ipv4/ip4_frag.c +++ b/src/core/ipv4/ip4_frag.c @@ -175,19 +175,21 @@ ip_reass_free_complete_datagram(struct ip_reassdata *ipr, struct ip_reassdata *p MIB2_STATS_INC(mib2.ipreasmfails); #if LWIP_ICMP - iprh = (struct ip_reass_helper *)ipr->p->payload; - if (iprh->start == 0) { - /* The first fragment was received, send ICMP time exceeded. */ - /* First, de-queue the first pbuf from r->p. */ - p = ipr->p; - ipr->p = iprh->next_pbuf; - /* Then, copy the original header into it. */ - SMEMCPY(p->payload, &ipr->iphdr, IP_HLEN); - icmp_time_exceeded(p, ICMP_TE_FRAG); - clen = pbuf_clen(p); - LWIP_ASSERT("pbufs_freed + clen <= 0xffff", pbufs_freed + clen <= 0xffff); - pbufs_freed = (u16_t)(pbufs_freed + clen); - pbuf_free(p); + if (ipr->p != NULL) { + iprh = (struct ip_reass_helper *)ipr->p->payload; + if (iprh->start == 0) { + /* The first fragment was received, send ICMP time exceeded. */ + /* First, de-queue the first pbuf from r->p. */ + p = ipr->p; + ipr->p = iprh->next_pbuf; + /* Then, copy the original header into it. */ + SMEMCPY(p->payload, &ipr->iphdr, IP_HLEN); + icmp_time_exceeded(p, ICMP_TE_FRAG); + clen = pbuf_clen(p); + LWIP_ASSERT("pbufs_freed + clen <= 0xffff", pbufs_freed + clen <= 0xffff); + pbufs_freed = (u16_t)(pbufs_freed + clen); + pbuf_free(p); + } } #endif /* LWIP_ICMP */ diff --git a/src/core/ipv6/ip6_frag.c b/src/core/ipv6/ip6_frag.c index ba91cfd4..66b12509 100644 --- a/src/core/ipv6/ip6_frag.c +++ b/src/core/ipv6/ip6_frag.c @@ -154,35 +154,37 @@ ip6_reass_free_complete_datagram(struct ip6_reassdata *ipr) struct ip6_reass_helper *iprh; #if LWIP_ICMP6 - iprh = (struct ip6_reass_helper *)ipr->p->payload; - if (iprh->start == 0) { - /* The first fragment was received, send ICMP time exceeded. */ - /* First, de-queue the first pbuf from r->p. */ - p = ipr->p; - ipr->p = iprh->next_pbuf; - /* Restore the part that we've overwritten with our helper structure, or we - * might send garbage (and disclose a pointer) in the ICMPv6 reply. */ - MEMCPY(p->payload, ipr->orig_hdr, sizeof(*iprh)); - /* Then, move back to the original ipv6 header (we are now pointing to Fragment header). - This cannot fail since we already checked when receiving this fragment. */ - if (pbuf_header_force(p, (s16_t)((u8_t*)p->payload - (u8_t*)ipr->iphdr))) { - LWIP_ASSERT("ip6_reass_free: moving p->payload to ip6 header failed", 0); + if (ipr->p != NULL) { + iprh = (struct ip6_reass_helper *)ipr->p->payload; + if (iprh->start == 0) { + /* The first fragment was received, send ICMP time exceeded. */ + /* First, de-queue the first pbuf from r->p. */ + p = ipr->p; + ipr->p = iprh->next_pbuf; + /* Restore the part that we've overwritten with our helper structure, or we + * might send garbage (and disclose a pointer) in the ICMPv6 reply. */ + MEMCPY(p->payload, ipr->orig_hdr, sizeof(*iprh)); + /* Then, move back to the original ipv6 header (we are now pointing to Fragment header). + This cannot fail since we already checked when receiving this fragment. */ + if (pbuf_header_force(p, (s16_t)((u8_t*)p->payload - (u8_t*)ipr->iphdr))) { + LWIP_ASSERT("ip6_reass_free: moving p->payload to ip6 header failed", 0); + } + else { + /* Reconstruct the zoned source and destination addresses, so that we do + * not end up sending the ICMP response over the wrong link. */ + ip6_addr_t src_addr, dest_addr; + ip6_addr_copy_from_packed(src_addr, IPV6_FRAG_SRC(ipr)); + ip6_addr_set_zone(&src_addr, ipr->src_zone); + ip6_addr_copy_from_packed(dest_addr, IPV6_FRAG_DEST(ipr)); + ip6_addr_set_zone(&dest_addr, ipr->dest_zone); + /* Send the actual ICMP response. */ + icmp6_time_exceeded_with_addrs(p, ICMP6_TE_FRAG, &src_addr, &dest_addr); + } + clen = pbuf_clen(p); + LWIP_ASSERT("pbufs_freed + clen <= 0xffff", pbufs_freed + clen <= 0xffff); + pbufs_freed = (u16_t)(pbufs_freed + clen); + pbuf_free(p); } - else { - /* Reconstruct the zoned source and destination addresses, so that we do - * not end up sending the ICMP response over the wrong link. */ - ip6_addr_t src_addr, dest_addr; - ip6_addr_copy_from_packed(src_addr, IPV6_FRAG_SRC(ipr)); - ip6_addr_set_zone(&src_addr, ipr->src_zone); - ip6_addr_copy_from_packed(dest_addr, IPV6_FRAG_DEST(ipr)); - ip6_addr_set_zone(&dest_addr, ipr->dest_zone); - /* Send the actual ICMP response. */ - icmp6_time_exceeded_with_addrs(p, ICMP6_TE_FRAG, &src_addr, &dest_addr); - } - clen = pbuf_clen(p); - LWIP_ASSERT("pbufs_freed + clen <= 0xffff", pbufs_freed + clen <= 0xffff); - pbufs_freed = (u16_t)(pbufs_freed + clen); - pbuf_free(p); } #endif /* LWIP_ICMP6 */