diff mbox

[Quantal] efivars: explicitly calculate length of VariableName

Message ID 1377574845-4470-1-git-send-email-hui.wang@canonical.com
State New
Headers show

Commit Message

Hui Wang Aug. 27, 2013, 3:40 a.m. UTC 
From: Matt Fleming <matt.fleming@intel.com>

BugLink: https://bugs.launchpad.net/hwe-sutton/+bug/1211725

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 <fcrozat@suse.com>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Josh Boyer <jwboyer@redhat.com>
Cc: Michael Schroeder <mls@suse.com>
Cc: Lee, Chun-Yi <jlee@suse.com>
Cc: Lingzhu Xiang <lxiang@redhat.com>
Cc: Seiji Aguchi <seiji.aguchi@hds.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
(cherry picked from commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a)

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
This patch was merged to upstream from linux-3.9-rc3, and the raring
kernel already backported this patch but quantal kernel didn't, so here
backport this patch to quantal kernel.

 drivers/firmware/efivars.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Luis Henriques Aug. 27, 2013, 10:12 a.m. UTC | #1 
Hui Wang <hui.wang@canonical.com> writes:

> From: Matt Fleming <matt.fleming@intel.com>
>
> BugLink: https://bugs.launchpad.net/hwe-sutton/+bug/1211725
>
> 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 <fcrozat@suse.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: Josh Boyer <jwboyer@redhat.com>
> Cc: Michael Schroeder <mls@suse.com>
> Cc: Lee, Chun-Yi <jlee@suse.com>
> Cc: Lingzhu Xiang <lxiang@redhat.com>
> Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> (cherry picked from commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a)
>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> This patch was merged to upstream from linux-3.9-rc3, and the raring
> kernel already backported this patch but quantal kernel didn't, so here
> backport this patch to quantal kernel.
>
>  drivers/firmware/efivars.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 3fb30ca..f51a759 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1439,6 +1439,32 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
>  	return count;
>  }
>  
> +
> +/*
> + * Returns the size of variable_name, in bytes, including the
> + * terminating NULL character, or variable_name_size if no NULL
> + * character is found among the first variable_name_size bytes.
> + */
> +static unsigned long var_name_strnsize(efi_char16_t *variable_name,
> +				       unsigned long variable_name_size)
> +{
> +	unsigned long len;
> +	efi_char16_t c;
> +
> +	/*
> +	 * The variable name is, by definition, a NULL-terminated
> +	 * string, so make absolutely sure that variable_name_size is
> +	 * the value we expect it to be. If not, return the real size.
> +	 */
> +	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
> +		c = variable_name[(len / sizeof(c)) - 1];
> +		if (!c)
> +			break;
> +	}
> +
> +	return min(len, variable_name_size);
> +}
> +
>  /*
>   * Let's not leave out systab information that snuck into
>   * the efivars driver
> @@ -1674,6 +1700,8 @@ int register_efivars(struct efivars *efivars,
>  						&vendor_guid);
>  		switch (status) {
>  		case EFI_SUCCESS:
> +			variable_name_size = var_name_strnsize(variable_name,
> +							       variable_name_size);
>  			efivar_create_sysfs_entry(efivars,
>  						  variable_name_size,
>  						  variable_name,
> -- 
> 1.8.1.2

[ Just a minor comment: you used 'cherry picked from commit ...'
  instead of 'back ported from commit ...' ]

The backport seems to make sense to me: you just dropped the chunk
related with the workqueue code (added by a93bc0c "efi_pstore:
Introducing workqueue updating sysfs").  This is also consistent with
the backport used by Greg for the 3.8 kernel.

Since I've backported to 3.5 stable kernel lots of EFI-related
commits, I'll also queue this one for next release.

Cheers,
Hui Wang Aug. 27, 2013, 10:17 a.m. UTC | #2 
On 08/27/2013 06:12 PM, Luis Henriques wrote:
> Hui Wang <hui.wang@canonical.com> writes:
>
>> From: Matt Fleming <matt.fleming@intel.com>
>>
>> BugLink: https://bugs.launchpad.net/hwe-sutton/+bug/1211725
>>
>> It's not wise to assume VariableNameSize represents the length of
>> VariableName, as not all firmware updates VariableNameSize in the same
[snip]
>>   						  variable_name_size,
>>   						  variable_name,
>> -- 
>> 1.8.1.2
> [ Just a minor comment: you used 'cherry picked from commit ...'
>    instead of 'back ported from commit ...' ]
>
> The backport seems to make sense to me: you just dropped the chunk
> related with the workqueue code (added by a93bc0c "efi_pstore:
> Introducing workqueue updating sysfs").  This is also consistent with
> the backport used by Greg for the 3.8 kernel.
>
> Since I've backported to 3.5 stable kernel lots of EFI-related
> commits, I'll also queue this one for next release.
>
> Cheers,
Got it, thanks.

Hui.
Andy Whitcroft Aug. 27, 2013, 10:39 a.m. UTC | #3 
On Tue, Aug 27, 2013 at 11:40:45AM +0800, Hui Wang wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> BugLink: https://bugs.launchpad.net/hwe-sutton/+bug/1211725
> 
> 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 <fcrozat@suse.com>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org>
> Cc: Josh Boyer <jwboyer@redhat.com>
> Cc: Michael Schroeder <mls@suse.com>
> Cc: Lee, Chun-Yi <jlee@suse.com>
> Cc: Lingzhu Xiang <lxiang@redhat.com>
> Cc: Seiji Aguchi <seiji.aguchi@hds.com>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> (cherry picked from commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a)
> 
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> This patch was merged to upstream from linux-3.9-rc3, and the raring
> kernel already backported this patch but quantal kernel didn't, so here
> backport this patch to quantal kernel.
> 
>  drivers/firmware/efivars.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 3fb30ca..f51a759 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1439,6 +1439,32 @@ static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
>  	return count;
>  }
>  
> +
> +/*
> + * Returns the size of variable_name, in bytes, including the
> + * terminating NULL character, or variable_name_size if no NULL
> + * character is found among the first variable_name_size bytes.
> + */
> +static unsigned long var_name_strnsize(efi_char16_t *variable_name,
> +				       unsigned long variable_name_size)
> +{
> +	unsigned long len;
> +	efi_char16_t c;
> +
> +	/*
> +	 * The variable name is, by definition, a NULL-terminated
> +	 * string, so make absolutely sure that variable_name_size is
> +	 * the value we expect it to be. If not, return the real size.
> +	 */
> +	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
> +		c = variable_name[(len / sizeof(c)) - 1];
> +		if (!c)
> +			break;
> +	}
> +
> +	return min(len, variable_name_size);
> +}
> +
>  /*
>   * Let's not leave out systab information that snuck into
>   * the efivars driver
> @@ -1674,6 +1700,8 @@ int register_efivars(struct efivars *efivars,
>  						&vendor_guid);
>  		switch (status) {
>  		case EFI_SUCCESS:
> +			variable_name_size = var_name_strnsize(variable_name,
> +							       variable_name_size);
>  			efivar_create_sysfs_entry(efivars,
>  						  variable_name_size,
>  						  variable_name,

Does seem to be missing one (not yet introduced fragment) so I assume
this is a backport.  Otherwise it seems to do what is claimed and ought
to be safe.  The original (new) code seems a bit pants, but matches what
is upstream.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw
Andy Whitcroft Aug. 27, 2013, 10:42 a.m. UTC | #4 
On Tue, Aug 27, 2013 at 11:40:45AM +0800, Hui Wang wrote:
> From: Matt Fleming <matt.fleming@intel.com>
> 
> BugLink: https://bugs.launchpad.net/hwe-sutton/+bug/1211725

When going to apply this I note that this above is a Private bug, we need a
public bug to do the validation testing against for the SRU process.

-apw
Hui Wang Aug. 28, 2013, 8:01 a.m. UTC | #5 
On 08/27/2013 06:42 PM, Andy Whitcroft wrote:
> On Tue, Aug 27, 2013 at 11:40:45AM +0800, Hui Wang wrote:
>> From: Matt Fleming <matt.fleming@intel.com>
>>
>> BugLink: https://bugs.launchpad.net/hwe-sutton/+bug/1211725
> When going to apply this I note that this above is a Private bug, we need a
> public bug to do the validation testing against for the SRU process.
Just filed a public bug.

BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1217745
> -apw
>
Tim Gardner Aug. 28, 2013, 1:31 p.m. UTC | #6 
Applied after appropriate commit log fixups, including bug number and
authorship.
diff mbox

Patch

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 3fb30ca..f51a759 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1439,6 +1439,32 @@  static ssize_t efivar_delete(struct file *filp, struct kobject *kobj,
 	return count;
 }
 
+
+/*
+ * Returns the size of variable_name, in bytes, including the
+ * terminating NULL character, or variable_name_size if no NULL
+ * character is found among the first variable_name_size bytes.
+ */
+static unsigned long var_name_strnsize(efi_char16_t *variable_name,
+				       unsigned long variable_name_size)
+{
+	unsigned long len;
+	efi_char16_t c;
+
+	/*
+	 * The variable name is, by definition, a NULL-terminated
+	 * string, so make absolutely sure that variable_name_size is
+	 * the value we expect it to be. If not, return the real size.
+	 */
+	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
+		c = variable_name[(len / sizeof(c)) - 1];
+		if (!c)
+			break;
+	}
+
+	return min(len, variable_name_size);
+}
+
 /*
  * Let's not leave out systab information that snuck into
  * the efivars driver
@@ -1674,6 +1700,8 @@  int register_efivars(struct efivars *efivars,
 						&vendor_guid);
 		switch (status) {
 		case EFI_SUCCESS:
+			variable_name_size = var_name_strnsize(variable_name,
+							       variable_name_size);
 			efivar_create_sysfs_entry(efivars,
 						  variable_name_size,
 						  variable_name,