Hi Jerome, Good catch — you're right, the outer bound alone doesn't cover it. dhcp_process_options() trusted `popt < end` to mean the whole option (code + length + payload) was safe, but that only guarantees the code byte is readable. oplen = *(popt + 1) and the payload dereferences in the switch had no bound check of their own, so a packet truncated right after an option code byte, or with a declared length that overruns the buffer, still walked off the end even with the outer fix applied.
Sent as v2 with a prerequisite patch (1/2) that fixes this: the pad option is now handled before touching a second byte, a length byte is required to exist before it's read, and the full declared option (header + payload) is required to fit before the switch runs on it — otherwise parsing stops instead of reading past the buffer. Your original outer-bound fix is 2/2, unchanged, rebased on top. Both commits are checkpatch --strict clean and build warning-free at W=1; a full sandbox build also passes. Thanks, Pranav On Thu, Aug 20, 2026 at 1:33 PM Pranav Rajendran <[email protected]> wrote: > dhcp_packet_process_options() derives the end of the option area from > BOOTP_HDR_SIZE, a compile-time constant, rather than from the length of > the packet that was actually received: > > uchar *popt = (uchar *)&bp->bp_vend[4]; > uchar *end = popt + BOOTP_HDR_SIZE; > > Since popt already starts near the end of the header, 'end' lands > sizeof(struct bootp_hdr) bytes beyond it, so a short reply leaves > dhcp_process_options() walking off the end of the received data and > into whatever the receive buffer held before - typically the remains of > earlier packets. > > That is not only a disclosure: the options found there are acted on > like any others, so stale bytes that happen to parse as an option can > influence the boot file name, the DNS server or the root path. > > The BOOTP path already gets this right and passes the real length to > bootp_process_vendor(), and both callers here have the length in scope > - they hand it to dhcp_message_type() on the lines above. Pass it in > and use it as the limit. The overloaded 'file' and 'sname' areas are > clamped the same way, as a truncated packet need not contain them > either. > > Fixes: 774c3e05ec0a ("net: parse DHCP options from overloaded file/sname > fields") > Signed-off-by: Pranav Rajendran <[email protected]> > --- > v2: No change, rebased on top of the new patch 1/2 which fixes the > inner option-length validation gap Jerome raised against v1. > > net/bootp.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) > > diff --git a/net/bootp.c b/net/bootp.c > index eafbe9e3bb4..06083092875 100644 > --- a/net/bootp.c > +++ b/net/bootp.c > @@ -977,32 +977,43 @@ static void dhcp_process_options(uchar *popt, uchar > *end) > } > } > > -static void dhcp_packet_process_options(struct bootp_hdr *bp) > +static void dhcp_packet_process_options(struct bootp_hdr *bp, unsigned > int len) > { > - uchar *popt = (uchar *)&bp->bp_vend[4]; > - uchar *end = popt + BOOTP_HDR_SIZE; > + uchar *pkt_end = (uchar *)bp + len; > + uchar *popt, *end; > + > + if (len < offsetof(struct bootp_hdr, bp_vend) + 4) > + return; > > if (net_read_u32((u32 *)&bp->bp_vend[0]) != > htonl(BOOTP_VENDOR_MAGIC)) > return; > > + popt = (uchar *)&bp->bp_vend[4]; > + > dhcp_option_overload = 0; > > /* > * The 'options' field MUST be interpreted first, 'file' next, > * 'sname' last. > */ > - dhcp_process_options(popt, end); > + dhcp_process_options(popt, pkt_end); > > if (dhcp_option_overload & OVERLOAD_FILE) { > popt = (uchar *)bp->bp_file; > end = popt + sizeof(bp->bp_file); > - dhcp_process_options(popt, end); > + if (end > pkt_end) > + end = pkt_end; > + if (popt < end) > + dhcp_process_options(popt, end); > } > > if (dhcp_option_overload & OVERLOAD_SNAME) { > popt = (uchar *)bp->bp_sname; > end = popt + sizeof(bp->bp_sname); > - dhcp_process_options(popt, end); > + if (end > pkt_end) > + end = pkt_end; > + if (popt < end) > + dhcp_process_options(popt, end); > } > } > > @@ -1133,7 +1144,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, > struct in_addr sip, > debug("got BOOTP response; transitioning > to BOUND\n"); > goto dhcp_got_bootp; > } > - dhcp_packet_process_options(bp); > + dhcp_packet_process_options(bp, len); > if (CONFIG_IS_ENABLED(EFI_LOADER) && > IS_ENABLED(CONFIG_NETDEVICES)) > efi_net_set_dhcp_ack(pkt, len); > @@ -1160,7 +1171,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, > struct in_addr sip, > > if (dhcp_message_type((u8 *)bp->bp_vend, (u8 *)pkt + len) > == DHCP_ACK) { > dhcp_got_bootp: > - dhcp_packet_process_options(bp); > + dhcp_packet_process_options(bp, len); > /* Store net params from reply */ > store_net_params(bp); > dhcp_state = BOUND; > -- > 2.50.1 (Apple Git-155) > >
