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

[Laszlo Ersek]

On 10/19/23 15:50, Pedro Falcato wrote:
> On Wed, Oct 18, 2023 at 6:24 PM Laszlo Ersek <lersek at 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 at kernel.org>
>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Signed-off-by: Laszlo Ersek <lersek at 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 :(

> Anyway, retroactive
> Acked-by: Pedro Falcato <pedro.falcato at 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. :) )

Cheers
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109808): https://edk2.groups.io/g/devel/message/109808
Mute This Topic: https://groups.io/mt/102044004/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-