Hi Ryan, [Replying from my personal address because for some reason my email is rejected by the Arm email servers: "messages to GMX.DE; are blocked by your organization [...]"]
On 5/20/26 23:59, Ryan Persée wrote: > When CONFIG_SERVERIP_FROM_PROXYDHCP is enabled, a ProxyDHCP server > responds with a zero yiaddr and supplies PXE-specific parameters such > as the TFTP server address. However, the bootfile name was only read > from the BOOTP legacy 'bp_file' header field. Modern ProxyDHCP servers > (e.g. dnsmasq, iPXE) advertise the bootfile via DHCP option 67 instead, > which was previously ignored. > > Furthermore, once the ProxyDHCP bootfile was stored, it could be silently > overwritten by a subsequent DHCP OFFER or ACK from the main DHCP server > if that packet also carried a bootfile (via option 67 or bp_file). > > This patch makes three minimal changes within the existing > CONFIG_SERVERIP_FROM_PROXYDHCP guards: > > 1. Add store_proxydhcp_bootfile(), a targeted helper that walks the DHCP > vendor options of a ProxyDHCP response and extracts only option 67. > All other options are deliberately ignored so that the ProxyDHCP > server cannot alter the client's network configuration (gateway, DNS, > etc.), which remains the main DHCP server's responsibility. > Option 67 takes precedence over the legacy bp_file header field; > bp_file is used as a fallback via the existing store_bootp_params(). > > 2. Add a !net_boot_file_name[0] guard in store_bootp_params() and in > dhcp_process_options() case 67 so that a bootfile already set by the > ProxyDHCP response is not overwritten by the main DHCP exchange. > > 3. Extend the ProxyDHCP wait condition in the SELECTING state to also > delay when the bootfile has not yet been received, mirroring the > existing delay for the server IP address. > > Signed-off-by: Ryan Persée <[email protected]> > --- > net/bootp.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 49 insertions(+), 3 deletions(-) > > diff --git a/net/bootp.c b/net/bootp.c > index 8976936b184..a16245cc7e8 100644 > --- a/net/bootp.c > +++ b/net/bootp.c > @@ -161,7 +161,11 @@ static void store_bootp_params(struct bootp_hdr *bp) > !(dhcp_option_overload & OVERLOAD_FILE) && > #endif > (strlen(bp->bp_file) > 0) && > - !net_boot_file_name_explicit) { > + !net_boot_file_name_explicit > +#if defined(CONFIG_SERVERIP_FROM_PROXYDHCP) > + && !net_boot_file_name[0] > +#endif > + ) { > copy_filename(net_boot_file_name, bp->bp_file, > sizeof(net_boot_file_name)); > } > @@ -935,7 +939,11 @@ static void dhcp_process_options(uchar *popt, uchar *end) > case 66: /* Ignore TFTP server name */ > break; > case 67: /* Bootfile option */ > - if (!net_boot_file_name_explicit) { > + if (!net_boot_file_name_explicit > +#if defined(CONFIG_SERVERIP_FROM_PROXYDHCP) > + && !net_boot_file_name[0] > +#endif > + ) { > size = truncate_sz("Bootfile", > sizeof(net_boot_file_name), > oplen); > @@ -1080,6 +1088,43 @@ static void dhcp_send_request_packet(struct bootp_hdr > *bp_offer) > net_send_packet(net_tx_packet, pktlen); > } > > +#if defined(CONFIG_SERVERIP_FROM_PROXYDHCP) > +/* > + * Extract bootfile name from DHCP option 67 in a ProxyDHCP response. > + * Only option 67 is read; all other options are intentionally ignored to > + * avoid letting the ProxyDHCP server alter the client's network config > + * (gateway, DNS, etc.) which is the main DHCP server's responsibility. > + */ > +static void store_proxydhcp_bootfile(struct bootp_hdr *bp) > +{ > + uchar *popt = (uchar *)&bp->bp_vend[4]; > + uchar *end = (uchar *)bp + BOOTP_HDR_SIZE; > + int oplen, size; > + > + if (net_read_u32((u32 *)&bp->bp_vend[0]) != htonl(BOOTP_VENDOR_MAGIC)) > + return; > + > + while (popt < end && *popt != 0xff) { > + if (*popt == 0) { > + popt++; > + continue; > + } I believe you need this to avoid a potential OOB read: if (popt + 1 >= end) break; > + oplen = *(popt + 1); > + if (popt + 2 + oplen > end) > + break; > + if (*popt == 67 && !net_boot_file_name_explicit && > + !net_boot_file_name[0]) { > + size = truncate_sz("Bootfile", > + sizeof(net_boot_file_name), oplen); > + memcpy(net_boot_file_name, popt + 2, size); > + net_boot_file_name[size] = 0; > + break; > + } > + popt += oplen + 2; > + } > +} > +#endif > + > /* > * Handle DHCP received packets. > */ > @@ -1100,6 +1145,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, > struct in_addr sip, > > if (net_read_ip(&bp->bp_yiaddr).s_addr == 0) { > #if defined(CONFIG_SERVERIP_FROM_PROXYDHCP) > + store_proxydhcp_bootfile(bp); > store_bootp_params(bp); > #endif > return; > @@ -1130,7 +1176,7 @@ static void dhcp_handler(uchar *pkt, unsigned dest, > struct in_addr sip, > efi_net_set_dhcp_ack(pkt, len); > > #if defined(CONFIG_SERVERIP_FROM_PROXYDHCP) > - if (!net_server_ip.s_addr) > + if (!net_server_ip.s_addr || !net_boot_file_name[0]) Shouldn't !net_boot_file_name[0] be (!net_boot_file_name_explicit && !net_boot_file_name[0])? > udelay(CONFIG_SERVERIP_FROM_PROXYDHCP_DELAY_MS * > 1000); > #endif /* CONFIG_SERVERIP_FROM_PROXYDHCP */ Other than that, LGTM. Thanks, -- Jerome
