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

  <filesystem type='mount' accessmode='passthrough'>
    <driver type='virtiofs'/>
    <binary path='/usr/libexec/virtiofsd'/>
    <source dir='.../virtio-fs-root'/>
    <target dir='VirtioFilesystem'/>
    <address type='pci' domain='0x0000'
     bus='0x00' slot='0x02' function='0x0'/>
    <boot order='1'/>
  </filesystem>

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