[PATCH] Ensure custom FS label is properly read

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Memory allocated via AllocatePool() is not necessarily initialized
to zero. Hence, the buffer may contain random garbage after the
read custom FS label up to its \0-terminated end.
Explicitly use AllocateZeroPool() to avoid this issue.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils.c b/utils.c
index bd7b8d7..ba9239b 100644
--- a/utils.c
+++ b/utils.c
@@ -82,7 +82,7 @@ CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
{
EFI_STATUS status;
EFI_FILE_HANDLE tmp;
- CHAR16 *buffer = AllocatePool(64);
+ CHAR16 *buffer = AllocateZeroPool(64);
UINTN buffsize = 63;

status = uefi_call_wrapper(
--
2.32.0

[Jan Kiszka]

This also obsoletes the "buffer[buffsize] = L'\0'", right?

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Jan Kiszka]

Hmm, not yet getting the full point of the change: Doesn't buffsize
contain on success the actually read amount of data? And that line above
will properly terminate after that. So where should the junk come from?

[Christian Storm]

Hi Jan,

> >> Memory allocated via AllocatePool() is not necessarily initialized
> >> to zero. Hence, the buffer may contain random garbage after the
> >> read custom FS label up to its \0-terminated end.
> >> Explicitly use AllocateZeroPool() to avoid this issue.
> >>
> >> Signed-off-by: Christian Storm <christi...@siemens.com>
> >> ---
> >> utils.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/utils.c b/utils.c
> >> index bd7b8d7..ba9239b 100644
> >> --- a/utils.c
> >> +++ b/utils.c
> >> @@ -82,7 +82,7 @@ CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
> >> {
> >> EFI_STATUS status;
> >> EFI_FILE_HANDLE tmp;
> >> - CHAR16 *buffer = AllocatePool(64);
> >> + CHAR16 *buffer = AllocateZeroPool(64);
> >> UINTN buffsize = 63;
> >>
> >> status = uefi_call_wrapper(
> >>
> >
> > This also obsoletes the "buffer[buffsize] = L'\0'", right?

Yes and no as this patch has fixed this bug's symptoms, not the cause,
see below.

> Hmm, not yet getting the full point of the change: Doesn't buffsize
> contain on success the actually read amount of data?

Yes.

> And that line above will properly terminate after that.

No. Read() returns the number of *bytes* read, so the above string
termination proper has to read
buffer[buffsize/2] = L'\0'
as buffer is CHAR16 and we're indexing CHAR16 here, not CHAR8.

> So where should the junk come from?

The string is terminated at a wrong position, possibly even beyond its
buffer length if the read string is long enough. In between the actual
string's end and this wrong termination, there could be garbage if the
allocated memory is not initialized to 0. This is why AllocateZeroPool()
has fixed the symptoms ― but didn't fix the potential out-of-bounds
indexing by buffer[buffsize] = L'\0'.


I'll send a V2 with buffer[buffsize/2] = L'\0' or do you prefer
the AllocateZeroPool() option?


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

OK - good that we talked. :)

>
> I'll send a V2 with buffer[buffsize/2] = L'\0' or do you prefer
> the AllocateZeroPool() option?

I would prefer the "termination at correct spot" option. Maybe do
"buffsize / sizeof(CHAR16)" or so to make it clearer.

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Read() returns the number of *bytes* read and this is verbatim
used to index the CHAR16 buffer without accomodation to CHAR16,
resulting in a wrong string termination position and potentially
an out-of-bounds write if the read input is sufficiently long.
Fix it by accomodating the bytes read to CHAR16.

Signed-off-by: Christian Storm <christi...@siemens.com>
---
utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils.c b/utils.c

index 78a149a..6238c8b 100644
--- a/utils.c
+++ b/utils.c
@@ -95,7 +95,7 @@ CHAR16 *get_volume_custom_label(EFI_FILE_HANDLE fh)
if (status != EFI_SUCCESS) {
return NULL;
}
- buffer[buffsize] = L'\0';
+ buffer[buffsize/sizeof(CHAR16)] = L'\0';
(VOID)uefi_call_wrapper(fh->Close, 1, tmp);
return buffer;
}
--
2.32.0

[Jan Kiszka]

Thanks, applied.