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

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

[Laszlo Ersek]

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.


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

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

[Gerd Hoffmann]

On Wed, Oct 18, 2023 at 07:24:34PM +0200, Laszlo Ersek 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>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>



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

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

[Laszlo Ersek]

On 10/19/23 08:28, Gerd Hoffmann wrote:
> On Wed, Oct 18, 2023 at 07:24:34PM +0200, Laszlo Ersek 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>
> 
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>

Merged via <https://github.com/tianocore/edk2/pull/4930>; commit
8abbf6d87e68.

Thanks!
Laszlo



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

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

[Pedro Falcato]

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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [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@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.

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@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Pedro Falcato]

On Thu, Oct 19, 2023 at 3:16 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/19/23 15:50, Pedro Falcato wrote:
> > 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.
>
> Ouch! I'm sorry. I've grown to (quite mechanically) consult
> $EDK_TOOLS_PATH/Scripts/GetMaintainer.py for the Cc list :(

No biggie :)

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

Of course :) Honestly, I'd be okay with either behavior *but* now that
the standard has been really loose with the language all these years,
I just think it makes sense to adopt a looser posture for this stuff.
It reminds me of when POSIX has/had really poorly defined interfaces
and they end up taking whatever behaviors are out in the wild.
In our case, you could keep strict IsDirectory() checks and be spec
compliant (if the language got tightened up around ->Open()); or be
loose and support shim (et al, certainly) but technically break the
spec.

But FWIW what we'll all end up doing is just adapting our protocol
implementations to support shim, isn't it? :P

To the RH folks, what's the easiest way of testing shim booting *with this bug*?

-- 
Pedro


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

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

[Gerd Hoffmann]

  Hi,
 
> To the RH folks, what's the easiest way of testing shim booting *with this bug*?

I've just passed the host ESP to the guest using virtiofs, libvirt xml
for that looks like this:

    <filesystem type='mount' accessmode='passthrough'>
      <driver type='virtiofs'/>
      <source dir='/boot/efi'/>
      <target dir='/boot/efi'/>
      <boot order='1'/>
      <address type='pci' domain='0x0000' bus='0x07' slot='0x00' function='0x0'/>
    </filesystem>

Then boot with an empty varstore and watch shim fail to read
EFI/$distro/BOOTX64.CSV

For the ext4 filesystem driver you probably need an ESP in ext4
filesystem format to test this.

take care,
  Gerd



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