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

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

[Heinrich Schuchardt]

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
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>
---
v2:
	check for non-ANSI character
---
 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 || c > 0x7f)
+			goto err;
+		value[i] = c;
+	}
+	value[4] = 0;
+	if (strict_strtoul(value, 16, &index))
+		index = 0xffff;
 err:
 	return index;
 }
--
2.30.0

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

[Joel Peshkin]

Hi Takahiro Akashi,

   The issue here is causing a failure in the EFI tests whenever the
compiler is checking to make sure the code is not overrunning the stack.
Fixing it is absolutely necessary.  To see this problem, please apply
https://patchwork.ozlabs.org/project/uboot/patch/20210209033607.70955-1-joel.peshkin at broadcom.com/
and then run the pytests on the sandbox build.  You will see that this
failure occurs during the EFI tests and that is the only portion of uboot
expressing such a  bug during the test suites.

Regards,

Joel



On Thu, Feb 11, 2021 at 3:05 AM Heinrich Schuchardt <xypron.glpk@gmx.de>
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
> 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>
> ---
> v2:
>         check for non-ANSI character
> ---
>  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 || c > 0x7f)
> +                       goto err;
> +               value[i] = c;
> +       }
> +       value[4] = 0;
> +       if (strict_strtoul(value, 16, &index))
> +               index = 0xffff;
>  err:
>         return index;
>  }
> --
> 2.30.0
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4209 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210303/ab4de7b4/attachment.bin>