Skip to contentSkip to conversation

efi_loader: fix off-by-one bug in efi_get_variable

PATCH Superseded Ivan Gorinov 3 participants 3 messages May 8, 2018 – May 9, 2018

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Ivan Gorinov

author
#0 lore raw

PATCH 1/1 efi_loader: fix off-by-one bug in efi_get_variable

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 <[email protected]>
---
 lib/efi_loader/efi_variable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
lib/efi_loader/efi_variable.c +2 -2
index 6c177da..d031338 100644
@@ -68,11 +68,11 @@ static const char *hex2mem(u8 *mem, const char *hexstr, int count)
 	do {
 		int nibble;
 
-		*mem = 0;
-
 		if (!count || !*hexstr)
 			break;
 
+		*mem = 0;
+
 		nibble = hex(*hexstr);
 		if (nibble < 0)
 			break;
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/1 efi_loader: fix off-by-one bug in efi_get_variable

Quoted 5 lines · Ivan Gorinov

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 <[email protected]>

Thanks to the memset right above the loop we can just remove the *mem = 0 line altogether, no?

Alex

Quoted 22 lines
---
  lib/efi_loader/efi_variable.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
lib/efi_loader/efi_variable.c +2 -2
index 6c177da..d031338 100644
@@ -68,11 +68,11 @@ static const char *hex2mem(u8 *mem, const char *hexstr, int count)
  	do {
  		int nibble;
 
-		*mem = 0;
-
  		if (!count || !*hexstr)
  			break;
 
+		*mem = 0;
+
  		nibble = hex(*hexstr);
  		if (nibble < 0)
  			break;
#2 lore raw

PATCH 1/1 efi_loader: fix off-by-one bug in efi_get_variable

In reply to Ivan Gorinov
Quoted 5 lines · Ivan Gorinov

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 <[email protected]>

Thanks for the patch.

There other issues we might want to fix:

If the blob has an uneven number of hexadecimal digits 2 N + 1 the function hex2mem is called with count = 2 N + 2. hex('\0') will return -1, hex2mem returns NULL, and the blob is happily considered as correct. We should create an error instead.

There is no need for the argument count at all as hexstr is '\0' terminated.

Quoted 9 lines
---
  lib/efi_loader/efi_variable.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
lib/efi_loader/efi_variable.c +0 -0
index 6c177da..d031338 100644

lib/efi_loader/efi_variable.c static const char *hex2mem(u8 *mem, const char *hexstr, int count) · line 75

Quoted 10 lines
lib/efi_loader/efi_variable.c +2 -2
@@ -68,11 +68,11 @@ static const char *hex2mem(u8 *mem, const char *hexstr, int count)
  	do {
  		int nibble;
 
-		*mem = 0;
-
  		if (!count || !*hexstr)
  			break;
 
+		*mem = 0;
+

Why should we have this line at all? We set *mem = nibble below.

Regards

Heinrich

Quoted 4 lines
lib/efi_loader/efi_variable.c +0 -0
  		nibble = hex(*hexstr);
  		if (nibble < 0)
  			break;