Fix file system browser hang when enrolling MOK from disk

[miczyg1]

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.

[vathpela]

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?

[miczyg1]

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):

1. Some ESP, SimpleFS protocol present, openVolume works, GetInfo implemented.
2. EXT4, SimpleFS protocol present, openVolume works, GetInfo NOT implemented.
3. EXT4, SimpleFS protocol present, openVolume works, GetInfo NOT implemented.
4. NTFS, SimpleFS protocol present, openVolume works, GetInfo NOT implemented.
5. Another ESP, SimpleFS protocol present, openVolume works, GetInfo implemented.

Before my PR the result would be:

1. Added to entries[0].
2. SKIPPED/NOT added to entries[1], hits continue and entries[1] stays uninitialized (random).
3. SKIPPED/NOT added to entries[2], hits continue and entries[2] stays uninitialized (random).
4. SKIPPED/NOT added to entries[3], hits continue and entries[3] stays uninitialized (random).
5. Added to 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.

[miczyg1]

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.

[desowin]

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.

[miczyg1]

@vathpela have I made it more clear possibly?

[miczyg1]

@vathpela ping

[miczyg1]

Anything I can do to help this PR land @vathpela ?

[pietrushnic]

@miczyg1, maybe we can bring this topic to the Linux Plumbers Conference?

[miczyg1]

@miczyg1, maybe we can bring this topic to the Linux Plumbers Conference?

Have you brought it?

[pietrushnic]

No. @aronowski, what do you think about this problem?