Incorrect EFI_FILE_PROTOCOL->Open() usage in fallback

[vathpela]

Laszlo reported this in a RHEL bugzilla ( https://bugzilla.redhat.com/show_bug.cgi?id=1966973 ), but I need to reference it here, so here's the story:

*** Description of problem:

In "fallback.c", in the following call tree:

find_boot_csv()
try_boot_csv()
read_file()
fh->Open()

the "fh" base handle, relative to which Open() is supposed to open a
file, is not a directory, but a regular file. This works with some
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL implementations (notably
FatPkg/EnhancedFatDxe from edk2), but not with others (for example,
OvmfPkg/VirtioFsDxe).

The passage of the UEFI spec that governs the expected behavior of
EFI_FILE_PROTOCOL (aka (*EFI_FILE_HANDLE)) in the above context is
itself buggy.

*** Version-Release number of selected component (if applicable):

• shim-unsigned-x64-15.4-4.el8_1 (dist-git commit 1999eeb52fd2)
• via shim-15.4-2.el8_1 (dist-git commit eb38a06891a3)

*** How reproducible:
100%

*** Steps to Reproduce:

1. Install an OVMF binary no earlier than edk2-stable202102, on the virt
   host.

2. Install libvirtd packages no earlier than v7.1.0, on the virt host.

3. Install a reasonably recent QEMU, on the virt host.

4. Define a new libvirt domain with the following XML fragment
   (customize the host-side location of the virtio-fs root directory as
   needed):

No other bootable device should be present (disk, cd-rom, network
card).

5. Using an installed (virtual or physical) RHEL-8.5 system as origin,
   recursively copy the "EFI" directory from "/boot/efi" to
   ".../virtio-fs-root" on the virt host:

   ssh rhel85 tar -C /boot/efi -c EFI | tar -C .../virtio-fs-root -x -v

6. Launch the domain.

*** Actual results:

shim logs:

Could not read file "\EFI\redhat\BOOTX64.CSV": Invalid Parameter
Could not process \EFI\redhat\BOOTX64.CSV: Invalid Parameter

** Expected results:

grub should be reached -- shim should launch grub.

*** Additional info:

starting with find_boot_csv() in "fallback.c", we find:

798 efi_status = fh->Open(fh, &fh2, bootarchcsv,
799 EFI_FILE_READ_ONLY, 0);
800 if (EFI_ERROR(efi_status) || fh2 == NULL) {
801 console_print(L"Couldn't open \EFI\%s\%s: %r\n",
802 dirname, bootarchcsv, efi_status);
803 } else {
804 efi_status = try_boot_csv(fh2, dirname, bootarchcsv); ----------+
805 fh2->Close(fh2); |
806 if (EFI_ERROR(efi_status)) |
807 console_print(L"Could not process \EFI\%s\%s: %r\n", |
808 dirname, bootarchcsv, efi_status); |
|
|
645 try_boot_csv(EFI_FILE_HANDLE fh, CHAR16 *dirname, CHAR16 *filename) <-------------------+
646 {
647 CHAR16 *fullpath = NULL;
648 UINT64 pathlen = 0;
649 EFI_STATUS efi_status;
650
651 efi_status = make_full_path(dirname, filename, &fullpath, &pathlen);
652 if (EFI_ERROR(efi_status))
653 return efi_status;
654
655 VerbosePrint(L"Found file "%s"\n", fullpath);
656
657 CHAR16 *buffer;
658 UINT64 bs;
659 efi_status = read_file(fh, fullpath, &buffer, &bs); -------------------+
660 if (EFI_ERROR(efi_status)) { |
661 console_print(L"Could not read file "%s": %r\n", |
662 fullpath, efi_status); |
|
|
134 read_file(EFI_FILE_HANDLE fh, CHAR16 *fullpath, CHAR16 **buffer, UINT64 *bs) <-+
135 {
136 EFI_FILE_HANDLE fh2;
137 EFI_STATUS efi_status;
138
139 efi_status = fh->Open(fh, &fh2, fullpath, EFI_FILE_READ_ONLY, 0);
140 if (EFI_ERROR(efi_status)) {
141 console_print(L"Couldn't open "%s": %r\n", fullpath, efi_status);

On line 798, the file "\EFI\redhat\BOOTX64.CSV" is opened successfully,
the new file handle is output in "fh2". "fh2" is then passed
try_boot_csv() as parameter "fh" (line 804 to line 645).

On line 659, "fh" (still the handle for "\EFI\redhat\BOOTX64.CSV") is
passed to read_file() as parameter "fh" (line 134).

On line 139, shim tries to open "fullpath" (also
"\EFI\redhat\BOOTX64.CSV") relative to the file handle "fh", which
stands for "\EFI\redhat\BOOTX64.CSV".

This is wrong: open regular files (as opposed to open directories)
cannot be used as base locations for opening new filesystem objects.
This is what the virito-fs driver reports with EFI_INVALID_PARAMETER.

Note that this problem originates from a bug in the UEFI specification.
The UEFI spec (v2.9) says, under section "13.5 File Protocol",

An EFI_FILE_PROTOCOL provides access to a file's or directory's
contents, and is also a reference to a location in the directory tree
of the file system in which the file resides. With any given file
handle, other files may be opened relative to this file's location,
yielding new file handles.

Now consider the following pathname:

\EFI\FOO

Assume that we have an open file handle (an EFI_FILE_PROTOCOL) that
refers to this filesystem object.

Now further assume that we open the following pathname:

BAR

relative to the open file handle. What is the expectation for the full
(absolute) pathname of the resultant object? Two choices:

\EFI\FOO\BAR [1]
\EFI\BAR [2]

The first option makes sense if the original file handle, \EFI\FOO,
stands for a directory. (That is, "FOO" is a subdirectory of "EFI".) And
in that case, the second option is wrong.

The second option makes sense, perhaps, if the original file handle,
\EFI\FOO, stands for a regular file. (That is, "FOO" is a regular file
in the "EFI" directory.) In that case, the first option is simply
undefined -- \EFI\FOO identifies a regular file, so it has no child
directory entries, such as \EFI\FOO\BAR. This means that when we attempt
to open a pathname relative to a regular file, what we could mean in the
best case would be a sibling, and not a child, of the last pathname
component.

To stress it again: the UEFI spec language implies a child relationship
for the relative pathname when the base file handle stands for an open
directory, and -- at best -- a sibling relationship when the base file
handle stands for an open regular file.

This inconsistency is the bug in the UEFI specification. The language I
quoted above, namely

With any given file handle, other files may be opened relative to this
file's location

is ill-defined for regular files. Even if we attempt to retrofit the
intended meaning from directories to regular files, the behavior will
not be consistent -- see the child vs. sibling interpretations,
respectively.

To put differently, given "fh->Open()", the expression

"the location of fh"

is ill-defined in the UEFI spec. If "fh" is a directory, then its
"location" is defined as "itself", in the usual sense of pathnames. But
if "fh" stands for a regular file, then its "location" is -- at best --
defined as its parent directory. Because of this, filesystem objects
relative to the "location of fh" are also ill-defined. It only becomes
well-defined if we exclude regular files as base handles.

I expressly investigated this UEFI spec bug while developing the
virtio-fs driver for OVMF, and the EFI_INVALID_PARAMETER return value is
intentional. For opening filesystem objects with the
EFI_FILE_PROTOCOL.Open() member function, the base handle must refer to
a directory; it must not refer to a regular file.

Regarding a shim fix: the try_boot_csv() function should be invoked with
a file handle to the root directory (= the volume root). Actually, the
"fh" handle in find_boot_csv() already stands for an open directory, so
passing that to try_boot_csv() should work too (given that the pathname
is an absolute one, so it doesn't matter what directory it is relative
to -- but it still cannot be relative to a regular file).

[lersek]

@vathpela Hello Peter -- I've been looking into this (I've had a bit of free time near the end of my Christmas vacation). The usage of file handles (EFI_FILE_PROTOCOL-pointers) in fallback.c is unfortunately generally messy. I could try a surgical fix just for the reported symptom, or a larger overhaul. What would be your preference?

Some things that stand out, as of commit 657b248:

• bad error handling patterns in find_boot_options(). The releasing of file handles upon error is duplicated all over the place, but even like that, there's a leak near the first fh2->Read() call. This error handling should be consolidated with cascading error labels / gotos at the end of the function.
• same function: SetPosition() is superfluous.
• same function: the fh* variable names are non-intuitive.
• same function: allocating a buffer for reading each directory entry separately is wasteful and complicates resource release upon error. A single buffer should exist, and only ever grow, across the loop.
• directory scanning is duplicated between find_boot_options() and find_boot_csv(). A general set of interfaces (such as opendir/readdir/closedir) should be factored out.
• AFAICT, the EFI_FILE_DIRECTORY check is duplicated (superfluous) in find_boot_csv(); find_boot_options() will have pre-checked that.
• read_file() currently gets an open file handle to the boot CSV. It does two things with fh: attempts to reopen the same file (!) by absolute path in a new fh2 handle, which is the direct issue reported in this ticket, just for getting the file size, and reads the file contents via the original fh handle. I don't understand why two file handles exist here at all, given that fh already keeps the boot CSV file open, so we could just use it for both getting the file size and for reading the contents. Eliminating fh2 altogether would fix the reported issue immediately as well, as a side-effect. Am I missing something about the need for opening fh2?

Thanks.

[lersek]

Also, it's not clear why find_boot_csv() even exists, i.e. why it scans the directory for the possible boot CSV names. Once we have the directory, why don't we just go ahead and immediately try to open the CSV files? We know their names in advance.

Connected to that, the get_file_size() function is redundant when considered together with the directory scanning in find_boot_csv(). In find_boot_csv(), we read through the directory, looking for the possible CSV files, by investigating EFI_FILE_INFO structures. Whenever we find bootcsv or bootarchcsv, we have their sizes in the same EFI_FILE_INFO buffer at once!

Even if we eliminate the directory scanning from find_boot_csv(), and so get_file_size() remains relevant, we can much simplify get_file_size(); no memory allocation needed. Use SetPosition() to seek to the end of the file, and then GetPosition() to learn the absolute file position.

[lersek]

BTW, related UEFI spec ticket: https://mantis.uefi.org/mantis/view.php?id=2367

[lersek]

@frozencemetery Hi Robbie, could you comment please? My main question is whether the shim maintainers would like me to

• ignore/abandon this ticket altogether,
• implement the surgical fix (which makes the code even less clean),
• clean up all the EFI_FILE_PROTOCOL usage (effectively a rewrite of the affected functions) and implement the fix.

Thanks.

[lersek]

Furthermore, directory scanning code, with arguably overlapping functionality (and also with its own bugs, unfortunately), already exists in lib/simple_file.c.

[TriMoon]

@vathpela
starting with find_boot_csv() in "fallback.c", we find:
....

I wish ppl would use code blocks when posting code... 🤕
It's unreadable as-is, because where does the code start/end and where does the poster start again 🤷‍♀️
Anyhow cary on all, just wanted to vent my frustration in this case...

[lersek]

I wish ppl would use code blocks when posting code... It's unreadable as-is, because where does the code start/end and where does the poster start again

I originally reported this issue at https://bugzilla.redhat.com/show_bug.cgi?id=1966973 ; there it was entirely obvious where the code started/ended and where I, the poster, started again; refer to https://bugzilla.redhat.com/show_bug.cgi?id=1966973#c0.

The formatting got lost with the re-filing of the RHBZ ticket in the upstream issue tracker.

What's frustrating is that github.com went with this ridiculous markdown syntax, when plain ASCII was 100% sufficient for reporting bugs with careful visual layout.

[lersek]

worked around in edk2 commit 8abbf6d87e68 ("OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file", 2023-10-19), after filing a ticket for the UEFI spec as well (Mantis#2367)