[PATCH 1/2] efi: Add new variable attributes

[PATCH 1/2] efi: Add new variable attributes

[Matthew Garrett]

More recent versions of the UEFI spec have added new attributes for
variables. Add them.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
Cc: stable@vger.kernel.org
---
 include/linux/efi.h |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 88ec806..ec45ccd 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -554,7 +554,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.10

[PATCH 2/2] efi: Validate UEFI boot variables

[Matthew Garrett]

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 <mjg@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/firmware/efivars.c |  182 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

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

Re: [PATCH 1/2] efi: Add new variable attributes

[Linus Torvalds]

Ok, both patches applied,

                Linus

On Mon, Apr 30, 2012 at 1:11 PM, Matthew Garrett <mjg@redhat.com> wrote:
> More recent versions of the UEFI spec have added new attributes for
> variables. Add them.

Re: [PATCH 2/2] efi: Validate UEFI boot variables

[Shea Levy]

Hi,

On 04/30/2012 04:11 PM, 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.

Any chance this will make it safe to use efibootmgr on Apple EFI 
firmware? I've been afraid to use it because I've read it can silently 
brick the device due to a mistake in efibootmgr. Obviously this won't 
correct that mistake, but with this applied should a successful variable 
set imply that the firmware wasn't bricked?

Cheers,
Shea Levy

Re: [PATCH 2/2] efi: Validate UEFI boot variables

[Matthew Garrett]

On Mon, Apr 30, 2012 at 08:00:30PM -0400, Shea Levy wrote:
> On 04/30/2012 04:11 PM, 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.
> 
> Any chance this will make it safe to use efibootmgr on Apple EFI
> firmware? I've been afraid to use it because I've read it can
> silently brick the device due to a mistake in efibootmgr. Obviously
> this won't correct that mistake, but with this applied should a
> successful variable set imply that the firmware wasn't bricked?

As far as I know that's been fixed since 
202f9d0a41809e3424af5f61489b48b622824aed - the problem wasn't 
efibootmgr, the problem was Apple's firmware overwriting itself.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/2] efi: Validate UEFI boot variables

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3055 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 2/2] efi: Validate UEFI boot variables

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

Re: [PATCH 2/2] efi: Validate UEFI boot variables

[Alan Cox]

On Thu, 16 Feb 2012 08:58:37 -0500
Matthew Garrett <mjg@redhat.com> 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 <mjg@redhat.com>

Other than pr_err() as a nitpick comemnt this looks good to me

Acked-by: Alan Cox <alan@linux.intel.com>

[PATCH 2/2] efi: Validate UEFI boot variables

[Matthew Garrett]

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 <mjg@redhat.com>
Cc: stable@kernel.org
---
 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