Hi Pranav,
On 16/08/2026 00:08, Pranav Rajendran 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]>
> ---
> net/bootp.c | 27 +++++++++++++++++++--------
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/net/bootp.c b/net/bootp.c
> index f0dc329d6e4..fdedecb3f50 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -968,32 +968,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);
This fixes the outer bound, but I don't think it is sufficient to make the
parser
safe for truncated packets.
dhcp_process_options() currently starts with:
while (popt < end && *popt != 0xff) {
oplen = *(popt + 1);
If the received packet ends with a single option code byte, popt < end is true
but
popt + 1 is already out of bounds.
There is also no validation that the complete option payload is present before
the
switch processes it.
So I think a prerequisite patch is needed to fix dhcp_process_options() first.
Thanks,
--
Jerome
>
> 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);
> }
> }
>
> @@ -1124,7 +1135,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);
> @@ -1151,7 +1162,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;
