Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen

["Pedro Falcato" <pedro.falcato@gmail.com>]

On Wed, Oct 18, 2023 at 12:20 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/18/23 12:33, Gerd Hoffmann wrote:
> > VirtiofsDxe throws an error in case the caller tries to open a file or
> > directory using an handle with is not a directory, claiming that opening
> > something relative to a file does not make sense.
> >
> > The claim is correct, but the code throws errors for both relative and
> > absolute paths.  Add a check to fix that.
> >
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  OvmfPkg/VirtioFsDxe/SimpleFsOpen.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> > index a13d4f6a1e2d..1729ea2f5cf2 100644
> > --- a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> > +++ b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> > @@ -397,7 +397,7 @@ VirtioFsSimpleFileOpen (
> >    // it cannot be implemented consistently with how a file is referred to
> >    // relative to a directory).
> >    //
> > -  if (!VirtioFsFile->IsDirectory) {
> > +  if (!VirtioFsFile->IsDirectory && FileName[0] != '\\') {
> >      DEBUG ((
> >        DEBUG_ERROR,
> >        ("%a: Label=\"%s\" CanonicalPathname=\"%a\" FileName=\"%s\": "
>
> It's nice to see this topic pop up on edk2-devel; apparently you started
> testing shim on top of virtio-fs. :)
>
> I have had the following patch in my local repo, on a separate branch,
> since April this year:
>
> > commit cb4a6d1664ea6cabd14d2af0e5d9abb114973870
> > Author: Laszlo Ersek <lersek@redhat.com>
> > Date:   Sat Apr 8 22:50:50 2023 +0200
> >
> >     OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file
> >
> >     Referring to a file relative to a regular file makes no sense (or at least
> >     it cannot be implemented consistently with how a file is referred to
> >     relative to a directory). VirtioFsSimpleFileOpen() has enforced this
> >     strictly since the beginning, and a few months ago I reported USWG Mantis
> >     ticket #2367 [1] too, for clearing up the related confusion in the UEFI
> >     spec.
> >
> >     Unfortunately, the shim boot loader contains such a bug [2] [3]. I don't
> >     believe the shim bug is ever going to be fixed. We can however relax the
> >     check in VirtioFsSimpleFileOpen() a bit: if the pathname that's being
> >     opened relative to a regular file is absolute, then the base file is going
> >     to be ignored anyway, so we can let the caller's bug slide. This happens
> >     to make shim work.
> >
> >     Why this matters: UEFI-bootable Linux installer ISOs tend to come with
> >     shim and grub in the embedded (ElTorito) FAT image (ESP). Sometimes you
> >     want to build upstream shim/grub binaries, but boot the same ISO
> >     otherwise. The fastest way for overriding the ESP for this purpose is to
> >     copy its original contents to a virtio filesystem, then overwrite the shim
> >     and grub binaries from the host side. Note that this is different from
> >     direct-booting a kernel (via fw_cfg); the point is to check whether the
> >     just-built shim and grub are able to boot the rest of the ISO.
> >
> >     [1] https://mantis.uefi.org/mantis/view.php?id=2367

What does the mantis ticket say/conclude? Yay for private bug trackers
that need corporate buy-in...

FWIW, Ext4Dxe does
[...]

  if (!Ext4FileIsDir (Current)) {
    return EFI_INVALID_PARAMETER;
  }

  // If the path starts with a backslash, we treat the root directory
as the base directory
  if (FileName[0] == L'\\') {
    FileName++;
    Current = Partition->Root;
  }

so if shim/other important UEFI apps have a bug, I may need to fix
this as well...

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109732): https://edk2.groups.io/g/devel/message/109732
Mute This Topic: https://groups.io/mt/102036263/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-