[Acked] [Quantal][PATCH] efivars: explicitly calculate length of VariableName

[Andy Whitcroft]

On Tue, Aug 27, 2013 at 11:40:45AM +0800, Hui Wang wrote:
> From: Matt Fleming <matt.fleming at 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 at suse.com>
> Cc: Matthew Garrett <mjg59 at srcf.ucam.org>
> Cc: Josh Boyer <jwboyer at redhat.com>
> Cc: Michael Schroeder <mls at suse.com>
> Cc: Lee, Chun-Yi <jlee at suse.com>
> Cc: Lingzhu Xiang <lxiang at redhat.com>
> Cc: Seiji Aguchi <seiji.aguchi at hds.com>
> Signed-off-by: Matt Fleming <matt.fleming at intel.com>
> (cherry picked from commit ec50bd32f1672d38ddce10fb1841cbfda89cfe9a)
> 
> Signed-off-by: Hui Wang <hui.wang at 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 at canonical.com>

-apw