From 05468774f015a15449e86a966336dfafbb60f28a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=BBygowski?= Date: Sat, 16 Dec 2023 12:45:42 +0100 Subject: [PATCH 1/3] lib/simple_file.c: Allocate zeroed pool for SimpleFS entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loop retrieving the SimpleFS volume labels and names may skip some volumes if either HandleProtocol or OpenVolume or GetInfo fails. Those skipped volumes would have uninitialized pointers to their names in the respective entries indices. This would lead to accessing random memory in console_select, because count_lines would not catch the holes with non-existing entries. On affected platforms the result is a hang of the MokManager while trying to enroll a key from disk. The issue has been triggered on a TianoCore EDK2 UEFIPayload based firmware for x86 platforms with additional filesystem drivers: ExFAT, NTFS, EXT2 and EXT4. Use AllocateZeroPool to ensure entries array will be initialized with NULL pointers. Handling the non-existing entries will be added in subsequent commits. Signed-off-by: Michał Żygowski --- lib/simple_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/simple_file.c b/lib/simple_file.c index f22852d4c..43b2f87a1 100644 --- a/lib/simple_file.c +++ b/lib/simple_file.c @@ -184,7 +184,7 @@ simple_volume_selector(CHAR16 **title, CHAR16 **selected, EFI_HANDLE *h) if (!count || !vol_handles) return EFI_NOT_FOUND; - entries = AllocatePool(sizeof(CHAR16 *) * (count+1)); + entries = AllocateZeroPool(sizeof(CHAR16 *) * (count+1)); if (!entries) return EFI_OUT_OF_RESOURCES; From c529e744c12971246ae9e3a13b416ddc8f4502c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=BBygowski?= Date: Sat, 16 Dec 2023 12:58:47 +0100 Subject: [PATCH 2/3] simple_file: Allow to form a volume name from DevicePath MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In case GetInfo of volume root fails, it is still possible to form a volume name from the DevicePath. Do not skip given SimpleFS volume handle and try to form a name from DevicePath. That way we do not lose some filesystems from file browser. This change already fixes the problem of a hanging platform when trying to enroll a key from disk. However, there is still a chance of having a non-contiguous array of entries, which will be fixed in next commit. Signed-off-by: Michał Żygowski --- lib/simple_file.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/simple_file.c b/lib/simple_file.c index 43b2f87a1..fc082bed1 100644 --- a/lib/simple_file.c +++ b/lib/simple_file.c @@ -208,10 +208,13 @@ simple_volume_selector(CHAR16 **title, CHAR16 **selected, EFI_HANDLE *h) efi_status = root->GetInfo(root, &EFI_FILE_SYSTEM_INFO_GUID, &size, fi); - if (EFI_ERROR(efi_status)) - continue; + /* If GetInfo fails, try to form a name from DevicePath. */ + if (EFI_ERROR(efi_status)){ + name = NULL; + } else { + name = fi->VolumeLabel; + } - name = fi->VolumeLabel; if (!name || StrLen(name) == 0 || StrCmp(name, L" ") == 0) name = DevicePathToStr(DevicePathFromHandle(vol_handles[i])); From a1c525ce96097ae1d8d704f312ecf24c3603b1a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=C5=BBygowski?= Date: Sat, 16 Dec 2023 13:01:29 +0100 Subject: [PATCH 3/3] simple_file: Use second variable to create filesystem entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If HandleProtocol or OpenVolume fails, the entries array will become non-contiguous, i.e. will have NULL pointers between valid volume names in the array. Because of that count_lines may return a lower number of entries than expected. As a result one may not browse all valid filesystems in the file explorer. Add a second index variable that will increment only on successfully created filesystem entries. As a result, count_lines should return proper length and there won't be any lost partitions or accesses to invalid entries. Signed-off-by: Michał Żygowski --- lib/simple_file.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/simple_file.c b/lib/simple_file.c index fc082bed1..6057f8835 100644 --- a/lib/simple_file.c +++ b/lib/simple_file.c @@ -170,7 +170,7 @@ simple_file_write_all(EFI_FILE *file, UINTN size, void *buffer) EFI_STATUS simple_volume_selector(CHAR16 **title, CHAR16 **selected, EFI_HANDLE *h) { - UINTN count, i; + UINTN count, i, j; EFI_HANDLE *vol_handles = NULL; EFI_STATUS efi_status; CHAR16 **entries; @@ -188,7 +188,7 @@ simple_volume_selector(CHAR16 **title, CHAR16 **selected, EFI_HANDLE *h) if (!entries) return EFI_OUT_OF_RESOURCES; - for (i = 0; i < count; i++) { + for (i = 0, j = 0; i < count; i++) { char buf[4096]; UINTN size = sizeof(buf); EFI_FILE_SYSTEM_INFO *fi = (void *)buf; @@ -218,12 +218,12 @@ simple_volume_selector(CHAR16 **title, CHAR16 **selected, EFI_HANDLE *h) if (!name || StrLen(name) == 0 || StrCmp(name, L" ") == 0) name = DevicePathToStr(DevicePathFromHandle(vol_handles[i])); - entries[i] = AllocatePool((StrLen(name) + 2) * sizeof(CHAR16)); - if (!entries[i]) + entries[j] = AllocatePool((StrLen(name) + 2) * sizeof(CHAR16)); + if (!entries[j]) break; - StrCpy(entries[i], name); + StrCpy(entries[j++], name); } - entries[i] = NULL; + entries[j] = NULL; val = console_select(title, entries, 0);