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

[Greg Kurz]

On Fri, 1 Jun 2018 09:35:38 +0200
Thomas Huth <thuth at redhat.com> wrote:

> On 01.06.2018 09:24, Greg Kurz wrote:
> > 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 ?  
> 
> Right. As the comment already says, this should (and currently can)
> never happen. This is just defensive coding, in case somebody ever
> messes up the TFTP code. In "normal" Linux userspace code, I'd put an
> assert() here instead - but we don't have support for assert() in SLOF,
> so I used this if-statement instead.

What could be an acceptable assert() in SLOF ? Print out a message and
do while (1) so that *someone* can try to debug ?

> 
>  Thomas