Re: [PATCH v2 2/2] net: bootp: bound DHCP option parsing by the received packet length

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