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

[Thomas Huth]

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.

 Thomas