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.gori...@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);
> >
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
