More recent versions of the UEFI spec have added new attributes for
variables. Add them.
Signed-off-by: Matthew Garrett <[email protected]>
Cc: [email protected]
---
include/linux/efi.h | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 37c3007..7cce0ea 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -510,7 +510,18 @@ extern int __init efi_setup_pcdp_console(char *);
#define EFI_VARIABLE_NON_VOLATILE 0x0000000000000001
#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x0000000000000002
#define EFI_VARIABLE_RUNTIME_ACCESS 0x0000000000000004
-
+#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x0000000000000008
+#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x0000000000000010
+#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x0000000000000020
+#define EFI_VARIABLE_APPEND_WRITE 0x0000000000000040
+
+#define EFI_VARIABLE_MASK (EFI_VARIABLE_NON_VOLATILE | \
+ EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+ EFI_VARIABLE_RUNTIME_ACCESS | \
+ EFI_VARIABLE_HARDWARE_ERROR_RECORD | \
+ EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | \
+ EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | \
+ EFI_VARIABLE_APPEND_WRITE)
/*
* The type of search to perform when calling boottime->locate_handle
*/
--
1.7.7.6
A common flaw in UEFI systems is a refusal to POST triggered by a malformed
boot variable. Once in this state, machines may only be restored by
reflashing their firmware with an external hardware device. While this is
obviously a firmware bug, the serious nature of the outcome suggests that
operating systems should filter their variable writes in order to prevent
a malicious user from rendering the machine unusable.
Signed-off-by: Matthew Garrett <[email protected]>
Cc: [email protected]
---
drivers/firmware/efivars.c | 182 ++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 182 insertions(+), 0 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d25599f..891e467 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -191,6 +191,176 @@ utf16_strncmp(const efi_char16_t *a, const efi_char16_t *b, size_t len)
}
}
+static bool
+validate_device_path(struct efi_variable *var, int match, u8 *buffer, int len)
+{
+ struct efi_generic_dev_path *node;
+ int offset = 0;
+
+ node = (struct efi_generic_dev_path *)buffer;
+
+ while (offset < len) {
+ offset += node->length;
+
+ if (offset > len)
+ return false;
+
+ if ((node->type == EFI_DEV_END_PATH ||
+ node->type == EFI_DEV_END_PATH2) &&
+ node->sub_type == EFI_DEV_END_ENTIRE)
+ return true;
+
+ node = (struct efi_generic_dev_path *)(buffer + offset);
+ }
+
+ /*
+ * If we're here then either node->length pointed past the end
+ * of the buffer or we reached the end of the buffer without
+ * finding a device path end node.
+ */
+ return false;
+}
+
+static bool
+validate_boot_order(struct efi_variable *var, int match, u8 *buffer, int len)
+{
+ /* An array of 16-bit integers */
+ if ((len % 2) != 0)
+ return false;
+
+ return true;
+}
+
+static bool
+validate_load_option(struct efi_variable *var, int match, u8 *buffer, int len)
+{
+ u16 filepathlength;
+ int i, desclength = 0;
+
+ /* Either "Boot" or "Driver" followed by four digits of hex */
+ for (i = match; i < match+4; i++) {
+ if (hex_to_bin(var->VariableName[i] & 0xff) < 0)
+ return true;
+ }
+
+ /* A valid entry must be at least 6 bytes */
+ if (len < 6)
+ return false;
+
+ filepathlength = buffer[4] | buffer[5] << 8;
+
+ /*
+ * There's no stored length for the description, so it has to be
+ * found by hand
+ */
+ desclength = utf16_strsize((efi_char16_t *)(buffer + 6), len) + 2;
+
+ /* Each boot entry must have a descriptor */
+ if (!desclength)
+ return false;
+
+ /*
+ * If the sum of the length of the description, the claimed filepath
+ * length and the original header are greater than the length of the
+ * variable, it's malformed
+ */
+ if ((desclength + filepathlength + 6) > len)
+ return false;
+
+ /*
+ * And, finally, check the filepath
+ */
+ return validate_device_path(var, match, buffer + desclength + 6,
+ filepathlength);
+}
+
+static bool
+validate_uint16(struct efi_variable *var, int match, u8 *buffer, int len)
+{
+ /* A single 16-bit integer */
+ if (len != 2)
+ return false;
+
+ return true;
+}
+
+static bool
+validate_ascii_string(struct efi_variable *var, int match, u8 *buffer, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ if (buffer[i] > 127)
+ return false;
+
+ if (buffer[i] == 0)
+ return true;
+ }
+
+ return false;
+}
+
+struct variable_validate {
+ char *name;
+ bool (*validate)(struct efi_variable *var, int match, u8 *data,
+ int len);
+};
+
+static const struct variable_validate variable_validate[] = {
+ { "BootNext", validate_uint16 },
+ { "BootOrder", validate_boot_order },
+ { "DriverOrder", validate_boot_order },
+ { "Boot*", validate_load_option },
+ { "Driver*", validate_load_option },
+ { "ConIn", validate_device_path },
+ { "ConInDev", validate_device_path },
+ { "ConOut", validate_device_path },
+ { "ConOutDev", validate_device_path },
+ { "ErrOut", validate_device_path },
+ { "ErrOutDev", validate_device_path },
+ { "Timeout", validate_uint16 },
+ { "Lang", validate_ascii_string },
+ { "PlatformLang", validate_ascii_string },
+ { "", NULL },
+};
+
+static bool
+validate_var(struct efi_variable *var, u8 *data, int len)
+{
+ int i;
+ u16 *unicode_name = var->VariableName;
+
+ for (i = 0; variable_validate[i].validate != NULL; i++) {
+ const char *name = variable_validate[i].name;
+ int match;
+
+ for (match = 0; ; match++) {
+ char c = name[match];
+ u16 u = unicode_name[match];
+
+ /* All special variables are plain ascii */
+ if (u > 127)
+ return true;
+
+ /* Wildcard in the matching name means we've matched */
+ if (c == '*')
+ return variable_validate[i].validate(var,
+ match, data, len);
+
+ /* Case sensitive match */
+ if (c != u)
+ break;
+
+ /* Reached the end of the string while matching */
+ if (!c)
+ return variable_validate[i].validate(var,
+ match, data, len);
+ }
+ }
+
+ return true;
+}
+
static efi_status_t
get_var_data_locked(struct efivars *efivars, struct efi_variable *var)
{
@@ -324,6 +494,12 @@ efivar_store_raw(struct efivar_entry *entry, const char *buf, size_t count)
return -EINVAL;
}
+ if ((new_var->Attributes & ~EFI_VARIABLE_MASK) != 0 ||
+ validate_var(new_var, new_var->Data, new_var->DataSize) == false) {
+ printk(KERN_ERR "efivars: Malformed variable content\n");
+ return -EINVAL;
+ }
+
spin_lock(&efivars->lock);
status = efivars->ops->set_variable(new_var->VariableName,
&new_var->VendorGuid,
@@ -626,6 +802,12 @@ static ssize_t efivar_create(struct file *filp, struct kobject *kobj,
if (!capable(CAP_SYS_ADMIN))
return -EACCES;
+ if ((new_var->Attributes & ~EFI_VARIABLE_MASK) != 0 ||
+ validate_var(new_var, new_var->Data, new_var->DataSize) == false) {
+ printk(KERN_ERR "efivars: Malformed variable content\n");
+ return -EINVAL;
+ }
+
spin_lock(&efivars->lock);
/*
--
1.7.7.6
On Thu, 16 Feb 2012 08:58:37 -0500
Matthew Garrett <[email protected]> wrote:
> A common flaw in UEFI systems is a refusal to POST triggered by a malformed
> boot variable. Once in this state, machines may only be restored by
> reflashing their firmware with an external hardware device. While this is
> obviously a firmware bug, the serious nature of the outcome suggests that
> operating systems should filter their variable writes in order to prevent
> a malicious user from rendering the machine unusable.
>
> Signed-off-by: Matthew Garrett <[email protected]>
Other than pr_err() as a nitpick comemnt this looks good to me
Acked-by: Alan Cox <[email protected]>
On Mon, 2012-04-30 at 16:11 -0400, Matthew Garrett wrote:
> A common flaw in UEFI systems is a refusal to POST triggered by a malformed
> boot variable. Once in this state, machines may only be restored by
> reflashing their firmware with an external hardware device. While this is
> obviously a firmware bug, the serious nature of the outcome suggests that
> operating systems should filter their variable writes in order to prevent
> a malicious user from rendering the machine unusable.
[...]
> +static bool
> +validate_device_path(struct efi_variable *var, int match, u8 *buffer,
> int len)
> +{
> + struct efi_generic_dev_path *node;
> + int offset = 0;
> +
> + node = (struct efi_generic_dev_path *)buffer;
> +
> + while (offset < len) {
> + offset += node->length;
> +
> + if (offset > len)
> + return false;
> +
> + if ((node->type == EFI_DEV_END_PATH ||
> + node->type == EFI_DEV_END_PATH2) &&
> + node->sub_type == EFI_DEV_END_ENTIRE)
> + return true;
> +
> + node = (struct efi_generic_dev_path *)(buffer + offset);
> + }
This validation is crap; you're not accounting for the size of the node
or invalid lengths. Try:
while (offset <= len - sizeof(*node) &&
node->length >= sizeof(*node) &&
node->length <= len - offset) {
offset += node->length;
if ((node->type == EFI_DEV_END_PATH ||
node->type == EFI_DEV_END_PATH2) &&
node->sub_type == EFI_DEV_END_ENTIRE)
return true;
node = (struct efi_generic_dev_path *)(buffer + offset);
}
[...]
> +static bool
> +validate_load_option(struct efi_variable *var, int match, u8 *buffer, int len)
> +{
> + u16 filepathlength;
> + int i, desclength = 0;
> +
> + /* Either "Boot" or "Driver" followed by four digits of hex */
> + for (i = match; i < match+4; i++) {
> + if (hex_to_bin(var->VariableName[i] & 0xff) < 0)
> + return true;
> + }
Isn't this slightly too restrictive? The '& 0xff' results in many
non-ASCII characters aliasing hex digits and potentially causes a
variable to be validated as if it was special. I would think the
correct condition is:
if (var->VariableName[i] > 127 ||
hex_to_bin(var->VariableName[i]) < 0)
Presumably the variable should also be ignored if there are any more
characters after the 4 hex digits?
> + /* A valid entry must be at least 6 bytes */
> + if (len < 6)
> + return false;
Surely 8 bytes - otherwise you don't even have space for the
description's null terminator.
> + filepathlength = buffer[4] | buffer[5] << 8;
> +
> + /*
> + * There's no stored length for the description, so it has to be
> + * found by hand
> + */
> + desclength = utf16_strsize((efi_char16_t *)(buffer + 6), len) + 2;
[...]
Second argument should be len - 6.
Ben.
--
Ben Hutchings
Design a system any fool can use, and only a fool will want to use it.
On Wed, May 02, 2012 at 04:55:06AM +0100, Ben Hutchings wrote:
> This validation is crap; you're not accounting for the size of the node
> or invalid lengths. Try:
Good catch.
> Isn't this slightly too restrictive? The '& 0xff' results in many
> non-ASCII characters aliasing hex digits and potentially causes a
> variable to be validated as if it was special. I would think the
> correct condition is:
Mm. Yeah, I guess that should probably be permitted.
> if (var->VariableName[i] > 127 ||
> hex_to_bin(var->VariableName[i]) < 0)
>
> Presumably the variable should also be ignored if there are any more
> characters after the 4 hex digits?
The spec doesn't permit any such names, so I don't think it makes any
difference in practice. But in theory, we should probably either
explicitly permit or reject them rather than treating them as if they're
boot variables.
> > + /* A valid entry must be at least 6 bytes */
> > + if (len < 6)
> > + return false;
>
> Surely 8 bytes - otherwise you don't even have space for the
> description's null terminator.
Yes, I guess that could trip things up.
> > + filepathlength = buffer[4] | buffer[5] << 8;
> > +
> > + /*
> > + * There's no stored length for the description, so it has to be
> > + * found by hand
> > + */
> > + desclength = utf16_strsize((efi_char16_t *)(buffer + 6), len) + 2;
> [...]
>
> Second argument should be len - 6.
Sure.
--
Matthew Garrett | [email protected]