Re: [PATCH] lib: Always NUL terminate ucs2_as_utf8

[Laszlo Ersek]

On 04/21/16 14:18, Matt Fleming wrote:
> ( Good Lord, I hate doing string manipulation in C )
> 
> On Wed, 20 Apr, at 03:25:32PM, Laszlo Ersek wrote:
>>
>> So, "len" does not include the room for the terminating NUL-byte here.
>> When "len" is passed to ucs2_as_utf8(), with the proposed patch applied,
>> a NUL byte will be produced in "name", but it will be at the price of a
>> genuine character from the input variable name.
> 
> Right, and this is a problem because we're trying to keep the names
> consistent between efivarfs and the EFI variable data. Force
> NUL-terminating the string is wrong, because if you have no room for
> the NUL the caller should check for that. Sadly none do.

I don't think it is necessary to check for the return value if on input the caller can guarantee (from ucs2_utf8size()) that the output buffer will be large enough.

> On the flip-side, passing around non-NUL terminated strings is just
> begging for these kinds of issues to come up.

"Depends", I guess :) NUL-termination and (ptr, length) both work, but whatever is chosen should be used consistently.

In my opinion, the pattern the code tries to follow is (ptr, length), and the direct issue is only that variable_matches() performs an out-of-bounds access.

We can change the pattern to NUL-termination, but then it should be consistently used, and I think variable_matches() should be adapted just the same. Why pass in the length to examine if the string is NUL-terminated, guaranteed?

> The fact is that the callers of ucs2_as_utf8() are passing it the
> wrong 'len' argument. We want a NUL-terminated utf8 string

Not necessarily. For example, efivarfs_callback() is constructing "name" in several steps, and ucs2_as_utf8() is just the first step. If it produced a '\0' at name[len], it would be overwritten soon after (with a hyphen character).

Also, the efivar_variable_is_removable() function takes "len". I think the interfaces are consistent, it's just the variable_matches() function that is buggy.

Again, I don't oppose switching the pattern to NUL-termination generally, but variable_matches() will have to be updated anyway. I think that's the first step, and the interfaces can be switched over only after.

> and we're
> passing a NUL-terminated ucs2 string. We should tell ucs2_as_utf8() it
> has enough room to copy the NUL.
> 
> Wouldn't this work (minus the return value checking)?
> 
> ---
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 0ac594c0a234..13a837c70e90 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -235,13 +235,12 @@ efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
>  	unsigned long utf8_size;
>  	u8 *utf8_name;
>  
> -	utf8_size = ucs2_utf8size(var_name);
> -	utf8_name = kmalloc(utf8_size + 1, GFP_KERNEL);
> +	utf8_size = ucs2_utf8size(var_name) + 1;
> +	utf8_name = kmalloc(utf8_size, GFP_KERNEL);
>  	if (!utf8_name)
>  		return false;
>  
>  	ucs2_as_utf8(utf8_name, var_name, utf8_size);
> -	utf8_name[utf8_size] = '\0';
>  
>  	for (i = 0; variable_validate[i].name[0] != '\0'; i++) {
>  		const char *name = variable_validate[i].name;
> @@ -250,7 +249,7 @@ efivar_validate(efi_guid_t vendor, efi_char16_t *var_name, u8 *data,
>  		if (efi_guidcmp(vendor, variable_validate[i].vendor))
>  			continue;
>  
> -		if (variable_matches(utf8_name, utf8_size+1, name, &match)) {
> +		if (variable_matches(utf8_name, utf8_size, name, &match)) {
>  			if (variable_validate[i].validate == NULL)
>  				break;
>  			kfree(utf8_name);

As I said, the thing I dislike about this is: why pass "utf8_size" to variable_matches() at all, if "utf8_name" is guaranteed to be NUL-terminated?

Otherwise, I think this patch does get the job done, yes. (I also checked the ucs2_as_utf8() call in efivar_create_sysfs_entry(), "drivers/firmware/efi/efivars.c", but there the buffer already has enough room for '\0'.)

... How about this instead?

> From 1684f4398b8498af135fb3e07f83614ef0423265 Mon Sep 17 00:00:00 2001
> From: Laszlo Ersek <lersek@xxxxxxxxxx>
> Date: Thu, 21 Apr 2016 18:08:31 +0200
> Subject: [PATCH] efi: fix out-of-bounds read in variable_matches()
>
> The variable_matches() function can currently read "var_name[len]", for
> example when:
> - var_name[0] == 'a',
> - len == 1
> - match_name points to the NUL-terminated string "ab".
>
> This function is supposed to accept "var_name" inputs that are not
> NUL-terminated (hence the "len" parameter"). Document the function, and
> access "var_name[*match]" only if "*match" is smaller than "len".
>
> Ref: http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/86906
> Reported-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Laszlo Ersek <lersek@xxxxxxxxxx>
> ---
>  drivers/firmware/efi/vars.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 0ac594c0a234..34b741940494 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -202,29 +202,44 @@ static const struct variable_validate variable_validate[] = {
>  	{ NULL_GUID, "", NULL },
>  };
>
> +/*
> + * Check if @var_name matches the pattern given in @match_name.
> + *
> + * @var_name: an array of @len non-NUL characters.
> + * @match_name: a NUL-terminated pattern string, optionally ending in "*". A
> + *              final "*" character matches any trailing characters @var_name,
> + *              including the case when there are none left in @var_name.
> + * @match: on output, the number of non-wildcard characters in @match_name
> + *         that @var_name matches, regardless of the return value.
> + * @return: whether @var_name fully matches @match_name.
> + */
>  static bool
>  variable_matches(const char *var_name, size_t len, const char *match_name,
>  		 int *match)
>  {
>  	for (*match = 0; ; (*match)++) {
>  		char c = match_name[*match];
> -		char u = var_name[*match];
>
> -		/* Wildcard in the matching name means we've matched */
> -		if (c == '*')
> +		switch (c) {
> +		case '*':
> +			/* Wildcard in @match_name means we've matched. */
>  			return true;
>
> -		/* Case sensitive match */
> -		if (!c && *match == len)
> -			return true;
> +		case '\0':
> +			/* @match_name has ended. Has @var_name too? */
> +			return (*match == len);
>
> -		if (c != u)
> +		default:
> +			/*
> +			 * We've reached a non-wildcard char in @match_name.
> +			 * Continue only if there's an identical character in
> +			 * @var_name.
> +			 */
> +			if (*match < len && c == var_name[*match])
> +				continue;
>  			return false;
> -
> -		if (!c)
> -			return true;
> +		}
>  	}
> -	return true;
>  }
>
>  bool
> --
> 1.8.3.1
>

Thanks
Laszlo

> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index dd029d13ea61..be5a02721b41 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -136,7 +136,7 @@ static int efivarfs_callback(efi_char16_t *name16, efi_guid_t vendor,
>  	if (!name)
>  		goto fail;
>  
> -	ucs2_as_utf8(name, entry->var.VariableName, len);
> +	ucs2_as_utf8(name, entry->var.VariableName, len + 1);
>  
>  	if (efivar_variable_is_removable(entry->var.VendorGuid, name, len))
>  		is_removable = true;
>