[PATCH 1/1] efi_loader: fix get_last_capsule()

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Tue, Feb 09, 2021 at 09:37:42PM +0100, Heinrich Schuchardt wrote:
> fix get_last_capsule() leads to writes beyond the stack allocated buffer.
> This was indicated when enabling the stack protector.
> 
> utf16_utf8_strcpy() only stops copying when reaching '\0'. The current
> invocation always writes beyond the end of value[].
> 
> The output length of utf16_utf8_strcpy() may be longer than the number of
> UTF-16 tokens. E.g has "Capsule????" has 11 UTF-16 tokens but 15 UTF-8

First of all, "CapsuleLast" is a read-only variable from user's viewpoint,
and is maintained solely by efi code. Then its value is expected to always
be sane.
The case you suggested above is quite unlikely.

Although I don't think we need this patch,

> tokens. Hence, using utf16_utf8_strcpy() without checking the input may
> lead to further writes beyond value[].
> 
> The current invocation of strict_strtoul() reads beyond the end of value[].
> 
> A non-hexadecimal value after "Capsule" (e.g. "CapsuleZZZZ") must result in
> an error. We cat catch this by checking the return value of strict_strtoul().
> 
> A value that is too short after "Capsule" (e.g. "Capsule0") must result in
> an error. We must check the string length of value[].
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_capsule.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index d39d731080..0017f0c0db 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -42,20 +42,28 @@ static struct efi_file_handle *bootdev_root;
>  static __maybe_unused unsigned int get_last_capsule(void)
>  {
>  	u16 value16[11]; /* "CapsuleXXXX": non-null-terminated */
> -	char value[11], *p;
> +	char value[5];
>  	efi_uintn_t size;
>  	unsigned long index = 0xffff;
>  	efi_status_t ret;
> +	int i;
> 
>  	size = sizeof(value16);
>  	ret = efi_get_variable_int(L"CapsuleLast", &efi_guid_capsule_report,
>  				   NULL, &size, value16, NULL);
> -	if (ret != EFI_SUCCESS || u16_strncmp(value16, L"Capsule", 7))
> +	if (ret != EFI_SUCCESS || size != 22 ||
> +	    u16_strncmp(value16, L"Capsule", 7))
>  		goto err;
> +	for (i = 0; i < 4; ++i) {
> +		u16 c = value16[i + 7];
> 
> -	p = value;
> -	utf16_utf8_strcpy(&p, value16);
> -	strict_strtoul(&value[7], 16, &index);
> +		if (!c)
> +			goto err;

Is this check necessary assuming size == 22?


> +		value[i] = c;

You are implicitly casting the value from u16 to u8 here.
This may lead to making an illegal code legal.

-Takahiro Akashi

> +	}
> +	value[4] = 0;
> +	if (strict_strtoul(value, 16, &index))
> +		index = 0xffff;
>  err:
>  	return index;
>  }
> --
> 2.30.0
>