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.g...@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
>
