* [U-Boot] [PATCH v3] efi_loader: fix off-by-one bug in efi_get_variable
@ 2018-05-11 17:54 Ivan Gorinov
2018-05-11 18:18 ` Heinrich Schuchardt
0 siblings, 1 reply; 3+ messages in thread
From: Ivan Gorinov @ 2018-05-11 17:54 UTC (permalink / raw)
To: u-boot
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@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);
- *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;
*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);
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* [U-Boot] [PATCH v3] efi_loader: fix off-by-one bug in efi_get_variable
2018-05-11 17:54 [U-Boot] [PATCH v3] efi_loader: fix off-by-one bug in efi_get_variable Ivan Gorinov
@ 2018-05-11 18:18 ` Heinrich Schuchardt
2018-05-11 20:14 ` Ivan Gorinov
0 siblings, 1 reply; 3+ messages in thread
From: Heinrich Schuchardt @ 2018-05-11 18:18 UTC (permalink / raw)
To: u-boot
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@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);
>
^ permalink raw reply [flat|nested] 3+ messages in thread* [U-Boot] [PATCH v3] efi_loader: fix off-by-one bug in efi_get_variable
2018-05-11 18:18 ` Heinrich Schuchardt
@ 2018-05-11 20:14 ` Ivan Gorinov
0 siblings, 0 replies; 3+ messages in thread
From: Ivan Gorinov @ 2018-05-11 20:14 UTC (permalink / raw)
To: u-boot
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@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);
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-05-11 20:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-11 17:54 [U-Boot] [PATCH v3] efi_loader: fix off-by-one bug in efi_get_variable Ivan Gorinov
2018-05-11 18:18 ` Heinrich Schuchardt
2018-05-11 20:14 ` Ivan Gorinov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox