Open miczyg1 opened 6 months ago
This looks fairly reasonable to me, but what I don't understand is this: what are the failures? How are they triggered, and under what conditions?
The FS drivers mentioned in my first comment did not necessarily implement GetInfo
method. This resulted in certain volumes to be skipped. As entries
array was not zero-pool allocation, the code executed later thought there are valid volumes/FSes in every entries
index, which was not always true. Given entry would be initialized to NULL only if entries[i] = AllocatePool((StrLen(name) + 2) * sizeof(CHAR16));
failed and returned NULL (breaking the loop additionally). But if not, the later code could access some random memory and the system would simply hang. That's the first problem.
The second problem is the multiple continue
directives in the loop. With many FSes in the system it becomes quite likely to skip some and create holes in the entries
array, but only fake holes because these skipped FSes could still become a random pointer in entries
array. But if for some reason we have hit if (!entries[i])
due to being unable to allocate pool for a volume name, we could prematurely terminate the loop and the entries
array limiting the possibility to browse all FSes - third problem (purely theoretical, not that we could do anything more when we cannot allocate memory anymore).
tl;dr Let's imagine 5 FSes (that's what I have been testing on):
Before my PR the result would be:
entries[0]
.entries[1]
, hits continue and entries[1]
stays uninitialized (random).entries[2]
, hits continue and entries[2]
stays uninitialized (random).entries[3]
, hits continue and entries[3]
stays uninitialized (random).entries[4]
entires[5] = NULL;
termination after the loop. Such array gets into console_select
and later hangs on accessing random pointer in entries[1]
, because count_lines
checks for NULL pointer termination and returns the length of 5.
Basically I have made the code more robust accounting for minimal FS driver implementations and avoiding possible access to random memory. Also the entries
array will contain only valid FSes not indexed by the SimpleFS handles in the buffer, but rather incrementally indexed by successfully parsed volumes/FSes.
I have encountered this exact issue on PRO Z790-P WIFI (MS-7E06) running Dasharo (coreboot+UEFI) v0.9.1 with disk where the only non-encrypted partition is EFI System Partition.
@vathpela have I made it more clear possibly?
@vathpela ping
Anything I can do to help this PR land @vathpela ?
@miczyg1, maybe we can bring this topic to the Linux Plumbers Conference?
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.