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

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

On Wed, Oct 18, 2023 at 1:20 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/18/23 13:33, Pedro Falcato wrote:
> > 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...
>
> I posted patches for the UEFI spec. (In two formats -- as a pull request
> to the locked-down repository on github.com, and as attachments.)
>
> Kevin W Shaw started reviewing my patches, but he seemed to
> misunderstand the git patch format in general; so I commented on that,
> but then the thread petered out. So it's stuck at the moment.
>
> I guess I could try to join USWG meetings / calls and champion the issue
> there, but I had not had time for that for a decade, and I don't have it
> now. I'd hope we could communicate asynchronously, via bug trackers...
>

Ok so I took some time to re-read the context and the spec. I think
you're going about things the wrong way.
The act of tolerating bad base "directories" in open calls is nothing
new. For instance, in POSIX:

int openat(int fd, const char *path, int oflag, ...);

[...]
The openat() function shall fail if:
[...]
[ENOTDIR] The path argument is not an absolute path and fd is a file
descriptor associated with a non-directory file.

so there's some track record in POSIX (in fact, openat is extra
tolerant and can also take bad file descriptors if the path is
absolute).

Given that there may be software out there that does rely on this, it
sure sounds like a better idea to be looser here, both in-driver and
in-spec.
What problem did GRUB have with this? GRUB freaking out with your
patch sure does seem like it would freak out with any run-of-the-mill
EnhancedFatDxe?

(FWIW, I've just checked and EnhancedFatDxe does not seem to even
check if the EFI_FILE is a directory, ever :))

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109745): https://edk2.groups.io/g/devel/message/109745
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]
-=-=-=-=-=-=-=-=-=-=-=-