* [PATCH 1/1] efi_loader: fix get_last_capsule()
@ 2021-02-09 20:37 Heinrich Schuchardt
2021-02-10 0:38 ` AKASHI Takahiro
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-02-09 20:37 UTC (permalink / raw)
To: u-boot
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>
---
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;
+ value[i] = c;
+ }
+ value[4] = 0;
+ if (strict_strtoul(value, 16, &index))
+ index = 0xffff;
err:
return index;
}
--
2.30.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/1] efi_loader: fix get_last_capsule()
2021-02-09 20:37 [PATCH 1/1] efi_loader: fix get_last_capsule() Heinrich Schuchardt
@ 2021-02-10 0:38 ` AKASHI Takahiro
2021-02-10 6:05 ` Heinrich Schuchardt
0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2021-02-10 0:38 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] efi_loader: fix get_last_capsule()
2021-02-10 0:38 ` AKASHI Takahiro
@ 2021-02-10 6:05 ` Heinrich Schuchardt
2021-02-10 6:43 ` AKASHI Takahiro
0 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-02-10 6:05 UTC (permalink / raw)
To: u-boot
Am 10. Februar 2021 01:38:38 MEZ schrieb 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.
What happens if the user tries to create a variable of the same name, e.g. in the variable store on disk?
>
>Although I don't think we need this patch,
Why do you think the patch is not necessary given that the current code is writing ouside buffers?
>
>> 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?
Ok. Instead we should check for > 0x7f here to avoid illegal codes.
Best regards
Heinrich
>
>
>> + 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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] efi_loader: fix get_last_capsule()
2021-02-10 6:05 ` Heinrich Schuchardt
@ 2021-02-10 6:43 ` AKASHI Takahiro
2021-02-10 6:49 ` Heinrich Schuchardt
2021-02-10 6:52 ` Heinrich Schuchardt
0 siblings, 2 replies; 6+ messages in thread
From: AKASHI Takahiro @ 2021-02-10 6:43 UTC (permalink / raw)
To: u-boot
On Wed, Feb 10, 2021 at 07:05:10AM +0100, Heinrich Schuchardt wrote:
> Am 10. Februar 2021 01:38:38 MEZ schrieb 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.
>
> What happens if the user tries to create a variable of the same name, e.g. in the variable store on disk?
If this is your concern, the best solution would be to prohibit
users to create system-managed (read-only) variables, like CapsuleLast.
If a user can create such a variable with an arbitrary value,
it will hurt the system's integrity.
>
> >
> >Although I don't think we need this patch,
>
> Why do you think the patch is not necessary given that the current code is writing ouside buffers?
My assumption here is, as I said,
> >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.
-Takahiro Akashi
> >
> >> 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?
>
> Ok. Instead we should check for > 0x7f here to avoid illegal codes.
>
> Best regards
>
> Heinrich
>
> >
> >
> >> + 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
> >>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] efi_loader: fix get_last_capsule()
2021-02-10 6:43 ` AKASHI Takahiro
@ 2021-02-10 6:49 ` Heinrich Schuchardt
2021-02-10 6:52 ` Heinrich Schuchardt
1 sibling, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-02-10 6:49 UTC (permalink / raw)
To: u-boot
Am 10. Februar 2021 07:43:25 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>On Wed, Feb 10, 2021 at 07:05:10AM +0100, Heinrich Schuchardt wrote:
>> Am 10. Februar 2021 01:38:38 MEZ schrieb 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.
>>
>> What happens if the user tries to create a variable of the same name,
>e.g. in the variable store on disk?
>
>If this is your concern, the best solution would be to prohibit
>users to create system-managed (read-only) variables, like CapsuleLast.
>
>If a user can create such a variable with an arbitrary value,
>it will hurt the system's integrity.
>
>>
>> >
>> >Although I don't think we need this patch,
>>
>> Why do you think the patch is not necessary given that the current
>code is writing ouside buffers?
>
>My assumption here is, as I said,
>> >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.
Even if the value is sane, the current code overwrites the stack of the caller? This becomes visible when the stack protector is enabled. Why don't you want to fix it?
Best regards
Heinrich
>
>-Takahiro Akashi
>
>
>> >
>> >> 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?
>>
>> Ok. Instead we should check for > 0x7f here to avoid illegal codes.
>>
>> Best regards
>>
>> Heinrich
>>
>> >
>> >
>> >> + 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
>> >>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] efi_loader: fix get_last_capsule()
2021-02-10 6:43 ` AKASHI Takahiro
2021-02-10 6:49 ` Heinrich Schuchardt
@ 2021-02-10 6:52 ` Heinrich Schuchardt
1 sibling, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-02-10 6:52 UTC (permalink / raw)
To: u-boot
Am 10. Februar 2021 07:43:25 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>On Wed, Feb 10, 2021 at 07:05:10AM +0100, Heinrich Schuchardt wrote:
>> Am 10. Februar 2021 01:38:38 MEZ schrieb 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.
>>
>> What happens if the user tries to create a variable of the same name,
>e.g. in the variable store on disk?
>
>If this is your concern, the best solution would be to prohibit
>users to create system-managed (read-only) variables, like CapsuleLast.
>
>If a user can create such a variable with an arbitrary value,
>it will hurt the system's integrity.
>
>>
>> >
>> >Although I don't think we need this patch,
>>
>> Why do you think the patch is not necessary given that the current
>code is writing ouside buffers?
>
>My assumption here is, as I said,
>> >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.
>
>-Takahiro Akashi
>
>
>> >
>> >> 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?
The value of bytes 19-22 could be zero.
>>
>> Ok. Instead we should check for > 0x7f here to avoid illegal codes.
>>
>> Best regards
>>
>> Heinrich
>>
>> >
>> >
>> >> + 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
>> >>
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-10 6:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09 20:37 [PATCH 1/1] efi_loader: fix get_last_capsule() Heinrich Schuchardt
2021-02-10 0:38 ` AKASHI Takahiro
2021-02-10 6:05 ` Heinrich Schuchardt
2021-02-10 6:43 ` AKASHI Takahiro
2021-02-10 6:49 ` Heinrich Schuchardt
2021-02-10 6:52 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.