Skip to content

Commit ec50bd3

Browse files
author
Matt Fleming
committed
efivars: explicitly calculate length of VariableName
It's not wise to assume VariableNameSize represents the length of VariableName, as not all firmware updates VariableNameSize in the same way (some don't update it at all if EFI_SUCCESS is returned). There are even implementations out there that update VariableNameSize with values that are both larger than the string returned in VariableName and smaller than the buffer passed to GetNextVariableName(), which resulted in the following bug report from Michael Schroeder, > On HP z220 system (firmware version 1.54), some EFI variables are > incorrectly named : > > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c The issue here is that because we blindly use VariableNameSize without verifying its value, we can potentially read garbage values from the buffer containing VariableName if VariableNameSize is larger than the length of VariableName. Since VariableName is a string, we can calculate its size by searching for the terminating NULL character. Reported-by: Frederic Crozat <[email protected]> Cc: Matthew Garrett <[email protected]> Cc: Josh Boyer <[email protected]> Cc: Michael Schroeder <[email protected]> Cc: Lee, Chun-Yi <[email protected]> Cc: Lingzhu Xiang <[email protected]> Cc: Seiji Aguchi <[email protected]> Signed-off-by: Matt Fleming <[email protected]>
1 parent ec0971b commit ec50bd3

File tree

1 file changed

+31
-1
lines changed

1 file changed

+31
-1
lines changed

drivers/firmware/efivars.c

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1705,6 +1705,31 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
17051705
return found;
17061706
}
17071707

1708+
/*
1709+
* Returns the size of variable_name, in bytes, including the
1710+
* terminating NULL character, or variable_name_size if no NULL
1711+
* character is found among the first variable_name_size bytes.
1712+
*/
1713+
static unsigned long var_name_strnsize(efi_char16_t *variable_name,
1714+
unsigned long variable_name_size)
1715+
{
1716+
unsigned long len;
1717+
efi_char16_t c;
1718+
1719+
/*
1720+
* The variable name is, by definition, a NULL-terminated
1721+
* string, so make absolutely sure that variable_name_size is
1722+
* the value we expect it to be. If not, return the real size.
1723+
*/
1724+
for (len = 2; len <= variable_name_size; len += sizeof(c)) {
1725+
c = variable_name[(len / sizeof(c)) - 1];
1726+
if (!c)
1727+
break;
1728+
}
1729+
1730+
return min(len, variable_name_size);
1731+
}
1732+
17081733
static void efivar_update_sysfs_entries(struct work_struct *work)
17091734
{
17101735
struct efivars *efivars = &__efivars;
@@ -1745,10 +1770,13 @@ static void efivar_update_sysfs_entries(struct work_struct *work)
17451770
if (!found) {
17461771
kfree(variable_name);
17471772
break;
1748-
} else
1773+
} else {
1774+
variable_name_size = var_name_strnsize(variable_name,
1775+
variable_name_size);
17491776
efivar_create_sysfs_entry(efivars,
17501777
variable_name_size,
17511778
variable_name, &vendor);
1779+
}
17521780
}
17531781
}
17541782

@@ -1995,6 +2023,8 @@ int register_efivars(struct efivars *efivars,
19952023
&vendor_guid);
19962024
switch (status) {
19972025
case EFI_SUCCESS:
2026+
variable_name_size = var_name_strnsize(variable_name,
2027+
variable_name_size);
19982028
efivar_create_sysfs_entry(efivars,
19992029
variable_name_size,
20002030
variable_name,

0 commit comments

Comments
 (0)