[U-Boot] [PATCH v3] efi_loader: fix off-by-one bug in efi_get_variable

[Ivan Gorinov]

On Fri, May 11, 2018 at 08:18:27PM +0200, Heinrich Schuchardt wrote:
> On 05/11/2018 07:54 PM, Ivan Gorinov wrote:
> > efi_get_variable() always stores an extra zero byte after the output data.
> > When the returned data size matches the output buffer size, the extra zero
> > byte is stored past the end of the output buffer.
> > 
> > Signed-off-by: Ivan Gorinov <ivan.gorinov at intel.com>
> > ---
> >  lib/efi_loader/efi_variable.c | 40 +++++++++++++++++-----------------------
> >  1 file changed, 17 insertions(+), 23 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 6c177da..28b2f5c 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -50,7 +50,7 @@
> >  	(strlen("efi_xxxxxxxx-xxxx-xxxx-xxxxxxxxxxxxxxxx_") + \
> >  		(MAX_VAR_NAME * MAX_UTF8_PER_UTF16))
> >  
> > -static int hex(unsigned char ch)
> > +static int hex(int ch)
> >  {
> >  	if (ch >= 'a' && ch <= 'f')
> >  		return ch-'a'+10;
> > @@ -61,44 +61,34 @@ static int hex(unsigned char ch)
> >  	return -1;
> >  }
> >  
> > -static const char *hex2mem(u8 *mem, const char *hexstr, int count)
> > +static int hex2mem(u8 *mem, const char *hexstr, int size)
> >  {
> > -	memset(mem, 0, count/2);
> > +	int nibble;
> > +	int i;
> >  
> > -	do {
> > -		int nibble;
> > +	memset(mem, 0, size);
> 
> Why should we call memset?
> 
> The memory is supplied by the caller of efi_get_variable().
> Either we copy size bytes to mem or we return EFI_DEVICE_ERROR.

OK.

> >  
> > -		*mem = 0;
> > -
> > -		if (!count || !*hexstr)
> > +	for (i = 0; i < size; i++) {
> > +		if (*hexstr == '\0')
> >  			break;
> >  
> >  		nibble = hex(*hexstr);
> >  		if (nibble < 0)
> > -			break;
> > +			return -1;
> >  
> >  		*mem = nibble;
> > -		count--;
> >  		hexstr++;
> >  
> > -		if (!count || !*hexstr)
> > -			break;
> > -
> >  		nibble = hex(*hexstr);
> >  		if (nibble < 0)
> > -			break;
> > +			return -1;
> >  
> >  		*mem = (*mem << 4) | nibble;
> > -		count--;
> >  		hexstr++;
> >  		mem++;
> > +	}
> >  
> > -	} while (1);
> > -
> > -	if (*hexstr)
> > -		return hexstr;
> > -
> > -	return NULL;
> > +	return i;
> >  }
> >  
> >  static char *mem2hex(char *hexstr, const u8 *mem, int count)
> > @@ -210,8 +200,12 @@ efi_status_t EFIAPI efi_get_variable(s16 *variable_name,
> >  	if ((s = prefix(val, "(blob)"))) {
> >  		unsigned len = strlen(s);
> >  
> > +		/* number of hexadecimal digits must be even */
> > +		if (len & 1)
> > +			return EFI_EXIT(EFI_DEVICE_ERROR);
> > +
> >  		/* two characters per byte: */
> > -		len = DIV_ROUND_UP(len, 2);
> > +		len /= 2;
> 
> You do not catch the case that len & 1 == 1 which should cause
> EFI_DEVICE_ERROR because the data in the blob is not valid.

This is exactly what new code in efi_get_variable() does:

	if (len & 1)
		return EFI_EXIT(EFI_DEVICE_ERROR);

> 
> Best regards
> 
> Heinrich
> 
> >  		*data_size = len;
> >  
> >  		if (in_size < len)
> > @@ -220,7 +214,7 @@ efi_status_t EFIAPI efi_get_variable(s16 *variable_name,
> >  		if (!data)
> >  			return EFI_EXIT(EFI_INVALID_PARAMETER);
> >  
> > -		if (hex2mem(data, s, len * 2))
> > +		if (hex2mem(data, s, len) != len)
> >  			return EFI_EXIT(EFI_DEVICE_ERROR);

Even if we did not check the string length early, hex2mem() would
return -1 when the number of digits is odd.

> >  
> >  		debug("%s: got value: \"%s\"\n", __func__, s);
> > 
>