From: "Pedro Falcato" <pedro.falcato@gmail.com>
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
Gerd Hoffmann <kraxel@redhat.com>,
Jiewen Yao <jiewen.yao@intel.com>,
Jordan Justen <jordan.l.justen@intel.com>
Subject: Re: [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file
Date: Thu, 19 Oct 2023 14:50:31 +0100 [thread overview]
Message-ID: <CAKbZUD153W+Eucq8SkSoSXfym1dELaddGgKhLm1i9-9bcgc4MQ@mail.gmail.com> (raw)
In-Reply-To: <20231018172434.91280-1-lersek@redhat.com>
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.
Anyway, retroactive
Acked-by: Pedro Falcato <pedro.falcato@gmail.com>
If this is the new pseudo-sanctioned behavior for filesystem drivers,
I'll make sure to do the same adjustments for Ext4Dxe.
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109805): https://edk2.groups.io/g/devel/message/109805
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]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2023-10-19 13:50 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-18 17:24 [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file Laszlo Ersek
2023-10-19 6:28 ` Gerd Hoffmann
2023-10-19 13:04 ` Laszlo Ersek
2023-10-19 13:50 ` Pedro Falcato [this message]
2023-10-19 14:16 ` Laszlo Ersek
2023-10-20 22:11 ` Pedro Falcato
2023-10-23 9:17 ` Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKbZUD153W+Eucq8SkSoSXfym1dELaddGgKhLm1i9-9bcgc4MQ@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox