Re: [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file

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

On Thu, Oct 19, 2023 at 3:16 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/19/23 15:50, Pedro Falcato wrote:
> > On Wed, Oct 18, 2023 at 6:24 PM Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> 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
> >> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1966973
> >> [3] https://github.com/rhboot/shim/issues/382
> >>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>
> >> Notes:
> >>     context:-U4
> >>
> >>  OvmfPkg/VirtioFsDxe/SimpleFsOpen.c | 17 ++++++++++++++---
> >>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >> index a13d4f6a1e2d..2ecf3d6c2325 100644
> >> --- a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >> +++ b/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c
> >> @@ -394,22 +394,33 @@ VirtioFsSimpleFileOpen (
> >>
> >>    //
> >>    // 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).
> >> +  // relative to a directory). See USWG Mantis ticket #2367.
> >>    //
> >>    if (!VirtioFsFile->IsDirectory) {
> >> +    BOOLEAN  BugCompat;
> >> +
> >> +    //
> >> +    // Tolerate this bug in the caller if FileName is absolute. If FileName is
> >> +    // absolute, then VirtioFsAppendPath() below will disregard
> >> +    // VirtioFsFile->CanonicalPathname.
> >> +    //
> >> +    BugCompat = (FileName[0] == L'\\');
> >> +
> >>      DEBUG ((
> >> -      DEBUG_ERROR,
> >> +      BugCompat ? DEBUG_WARN : DEBUG_ERROR,
> >>        ("%a: Label=\"%s\" CanonicalPathname=\"%a\" FileName=\"%s\": "
> >>         "nonsensical request to open a file or directory relative to a regular "
> >>         "file\n"),
> >>        __func__,
> >>        VirtioFs->Label,
> >>        VirtioFsFile->CanonicalPathname,
> >>        FileName
> >>        ));
> >> -    return EFI_INVALID_PARAMETER;
> >> +    if (!BugCompat) {
> >> +      return EFI_INVALID_PARAMETER;
> >> +    }
> >>    }
> >>
> >>    //
> >>    // Allocate the new VIRTIO_FS_FILE object.
> >>
> >
> > Aww, you should've CC'd me.
>
> Ouch! I'm sorry. I've grown to (quite mechanically) consult
> $EDK_TOOLS_PATH/Scripts/GetMaintainer.py for the Cc list :(

No biggie :)

>
> > Anyway, retroactive
> > Acked-by: Pedro Falcato <pedro.falcato@gmail.com>
>
> Thank you! (I've pushed the patch already, so can't record your A-b on
> it, but the mailing list will preserve it at least.)
>
> >
> > If this is the new pseudo-sanctioned behavior for filesystem drivers,
> > I'll make sure to do the same adjustments for Ext4Dxe.
> >
>
> I think that would be useful.
>
> (Of course I don't "insist" that you share my opinion on whether this
> new behavior is for bug compatibility with the UEFI spec and shim (which
> I think it is), or because it is the right/intuitive thing to do (which
> I don't think it is) -- just feel entirely free to word the Ext4Pkg
> comments and commit message as you see fit! ... And I probably don't
> even have to state this, but let me just state it anyway. :) )

Of course :) Honestly, I'd be okay with either behavior *but* now that
the standard has been really loose with the language all these years,
I just think it makes sense to adopt a looser posture for this stuff.
It reminds me of when POSIX has/had really poorly defined interfaces
and they end up taking whatever behaviors are out in the wild.
In our case, you could keep strict IsDirectory() checks and be spec
compliant (if the language got tightened up around ->Open()); or be
loose and support shim (et al, certainly) but technically break the
spec.

But FWIW what we'll all end up doing is just adapting our protocol
implementations to support shim, isn't it? :P

To the RH folks, what's the easiest way of testing shim booting *with this bug*?

-- 
Pedro


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