* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox