[SLOF] [PATCH 1/2] lib/libnet/pxelinux: Make the size handling for pxelinux_load_cfg more logical

[Greg Kurz]

On Fri,  1 Jun 2018 07:26:50 +0200
Thomas Huth <thuth at redhat.com> wrote:

> The pxelinux_load_cfg() function always tried to load one byte less than
> its parameter said (so that we've got space for a terminating NUL-character
> later). This is not very intuitive, let's better ask for one byte less
> when we call the function.

It's better indeed.

> While we're at it, add a sanity check that
> the function really did not load more bytes than requested.
> 

No sure for this... see below.

> Signed-off-by: Thomas Huth <thuth at redhat.com>
> ---
>  lib/libnet/pxelinux.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c
> index 718ceca..eaead48 100644
> --- a/lib/libnet/pxelinux.c
> +++ b/lib/libnet/pxelinux.c
> @@ -95,7 +95,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui
>  			return -1;
>  		}
>  		strcpy(baseptr, fn_ip->pl_cfgfile);
> -		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries);
> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries);
>  		if (rc > 0) {
>  			return rc;
>  		}
> @@ -104,7 +104,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui
>  	/* Try to load config file with name based on the VM UUID */
>  	if (uuid) {
>  		strcpy(baseptr, uuid);
> -		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries);
> +		rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries);
>  		if (rc > 0) {
>  			return rc;
>  		}
> @@ -113,7 +113,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui
>  	/* Look for config file with MAC address in its name */
>  	sprintf(baseptr, "01-%02x-%02x-%02x-%02x-%02x-%02x",
>  		mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
> -	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries);
> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries);
>  	if (rc > 0) {
>  		return rc;
>  	}
> @@ -127,7 +127,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui
>  			fn_ip->own_ip & 0xff);
>  		for (idx = 0; idx <= 7; idx++) {
>  			baseptr[8 - idx] = 0;
> -			rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1,
> +			rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize,
>  			                        retries);
>  			if (rc > 0) {
>  				return rc;
> @@ -137,7 +137,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui
>  
>  	/* Try "default" config file */
>  	strcpy(baseptr, "default");
> -	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries);
> +	rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries);
>  
>  	return rc;
>  }
> @@ -240,9 +240,13 @@ int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid
>  {
>  	int rc;
>  
> -	rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize);
> +	rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize - 1);
>  	if (rc < 0)
>  		return rc;
> +	if (rc >= cfgsize) {	/* This should never happen */

Can this even happen at all ? I see tftp() seem to have some code that would
cause pxelinux_load_cfg() to return -2 if the buffer length is reached... and
anyway, if it didn't then rc >= cfgsize would mean something was written past
the buffer or am I missing something... how can we cope with that ?

> +		puts("Downloaded pxelinux.cfg is too big!");
> +		return -1;
> +	}
>  
>  	return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent);
>  }