[PATCH v2 1/3] net: Get pxe config file from dhcp option 209

[Heinrich Schuchardt]

On 10/24/23 02:21, seanedmond at linux.microsoft.com wrote:
> From: Sean Edmond <seanedmond at microsoft.com>
>
> Allow dhcp server pass pxe config file full path by using option 209
>
> Signed-off-by: Sean Edmond <seanedmond at microsoft.com>
> ---
>   cmd/Kconfig |  4 ++++
>   cmd/pxe.c   | 10 ++++++++++
>   net/bootp.c | 21 +++++++++++++++++++++
>   3 files changed, 35 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5bc0a92d57..adbb1a6187 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1826,6 +1826,10 @@ config BOOTP_PXE_CLIENTARCH
>   	default 0x15 if ARM
>   	default 0x0 if X86
>
> +config BOOTP_PXE_DHCP_OPTION
> +	bool "Request & store 'pxe_configfile' from BOOTP/DHCP server"
> +	depends on BOOTP_PXE

Why should this be disabled by default?

Do we really want a separate config variable?

> +
>   config BOOTP_VCI_STRING
>   	string
>   	depends on CMD_BOOTP
> diff --git a/cmd/pxe.c b/cmd/pxe.c
> index 704589702f..9404f44518 100644
> --- a/cmd/pxe.c
> +++ b/cmd/pxe.c
> @@ -65,6 +65,8 @@ static int pxe_dhcp_option_path(struct pxe_context *ctx, unsigned long pxefile_a
>   	int ret = get_pxe_file(ctx, pxelinux_configfile, pxefile_addr_r);
>
>   	free(pxelinux_configfile);
> +	/* set to NULL to avoid double-free if DHCP is tried again */
> +	pxelinux_configfile = NULL;
>
>   	return ret;
>   }
> @@ -141,6 +143,14 @@ int pxe_get(ulong pxefile_addr_r, char **bootdirp, ulong *sizep, bool use_ipv6)
>   			  env_get("bootfile"), use_ipv6))
>   		return -ENOMEM;
>
> +	if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION) &&
> +	    pxelinux_configfile && !use_ipv6) {
> +		if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
> +			goto done;
> +
> +		goto error_exit;
> +	}
> +
>   	if (IS_ENABLED(CONFIG_DHCP6_PXE_DHCP_OPTION) &&
>   	    pxelinux_configfile && use_ipv6) {
>   		if (pxe_dhcp_option_path(&ctx, pxefile_addr_r) > 0)
> diff --git a/net/bootp.c b/net/bootp.c
> index 7b0f45e18a..6800290963 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -26,6 +26,7 @@
>   #ifdef CONFIG_BOOTP_RANDOM_DELAY
>   #include "net_rand.h"
>   #endif
> +#include <malloc.h>
>
>   #define BOOTP_VENDOR_MAGIC	0x63825363	/* RFC1048 Magic Cookie */
>
> @@ -601,6 +602,10 @@ static int dhcp_extended(u8 *e, int message_type, struct in_addr server_ip,
>   	*e++  = 42;
>   	*cnt += 1;
>   #endif
> +	if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
> +		*e++ = 209;	/* PXELINUX Config File */
> +		*cnt += 1;
> +	}
>   	/* no options, so back up to avoid sending an empty request list */
>   	if (*cnt == 0)
>   		e -= 2;
> @@ -909,6 +914,22 @@ static void dhcp_process_options(uchar *popt, uchar *end)
>   				net_boot_file_name[size] = 0;
>   			}
>   			break;
> +		case 209:	/* PXELINUX Config File */

This 209 appears in multiple places. Please, define a constant, e.g.
DHCP_OPTION_CONFIG_FILE, in bootp.h and use the symbolic name. Please,
document the constant referring to RFC 5071.

> +			if (IS_ENABLED(CONFIG_BOOTP_PXE_DHCP_OPTION)) {
> +				/* In case it has already been allocated when get DHCP Offer packet,
> +				 * free first to avoid memory leak.
> +				 */
> +				if (pxelinux_configfile)
> +					free(pxelinux_configfile);
> +
> +				pxelinux_configfile = (char *)malloc((oplen + 1) * sizeof(char));
> +
> +				if (pxelinux_configfile)
> +					strlcpy(pxelinux_configfile, popt + 2, oplen + 1);
> +				else
> +					printf("Error: Failed to allocate pxelinux_configfile\n");

We do the same in dhcp6_parse_options(). Please, factor out a common
function to avoid code duplication.

Best regards

Heinrich

> +			}
> +			break;
>   		default:
>   #if defined(CONFIG_BOOTP_VENDOREX)
>   			if (dhcp_vendorex_proc(popt))