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

["Laszlo Ersek" <lersek@redhat.com>]

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...

> 
> 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...
> 

I vaguely remember that I looked up both Ext4Dxe and EnhancedFatDxe
regarding this question, but I don't remember what my take-away was at
the time. :)

Clearly, EnhancedFatDxe must be tolerant of this bug, otherwise shim
would never boot off of "normal" (i.e., FAT32) ESPs.

Laszlo



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