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.aka...@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.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? > > 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 > >> >