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

[Heinrich Schuchardt]

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.

>  
> -		*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.

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);
>  
>  		debug("%s: got value: \"%s\"\n", __func__, s);
>