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 | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

lib/efi_loader/efi_variable.c +1 -1

index 6c177da..28b2f5c 100644
@@ -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;
 }
 

lib/efi_loader/efi_variable.c +6 -16

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

lib/efi_loader/efi_variable.c +1 -1

 		*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);

Quoted 33 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]>

---
 lib/efi_loader/efi_variable.c | 40 +++++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 23 deletions(-)

lib/efi_loader/efi_variable.c +4 -3

index 6c177da..28b2f5c 100644
@@ -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;
 

lib/efi_loader/efi_variable.c static int hex(unsigned char ch) · line 69

Quoted 3 lines

lib/efi_loader/efi_variable.c +1 -2

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

Quoted 45 lines

lib/efi_loader/efi_variable.c +6 -16

-		*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);
 

lib/efi_loader/efi_variable.c efi_status_t EFIAPI efi_get_variable(s16 *variable_name, · line 208

Quoted 7 lines

lib/efi_loader/efi_variable.c +5 -1

+		/* 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

Quoted 2 lines

lib/efi_loader/efi_variable.c +0 -0

 		*data_size = len;
 

lib/efi_loader/efi_variable.c efi_status_t EFIAPI efi_get_variable(s16 *variable_name, · line 218

Quoted 8 lines

lib/efi_loader/efi_variable.c +1 -1

 		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);

Quoted 3 lines

lib/efi_loader/efi_variable.c +0 -0

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