On 10/18/23 13:33, Pedro Falcato wrote:
> On Wed, Oct 18, 2023 at 12:20 PM Laszlo Ersek <ler...@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 <kra...@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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
