From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 99A51941598 for ; Wed, 18 Oct 2023 13:08:34 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=o9lhZ45YSGoH3bw7bKfPXjxZd9jVayBNfrI+J2TLpQw=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Transfer-Encoding; s=20140610; t=1697634513; v=1; b=ULCFS7ybJn1FHMRj0jNUtV8NVFlZKTfeAzUjNJx8c6xHceI6h5CE7qgOOVpM5h9QcePLjG22 eq+35PVHU6/h4zzhR+U6aSUcsD49+7qm9EyTvYY02ZNJFkB7iju7WKsc89BIZvIkS/DYTCio+P5 z+QEHGzWOTGEypFXOKjvMxm0= X-Received: by 127.0.0.2 with SMTP id ENZfYY7687511xmSETViwhHw; Wed, 18 Oct 2023 06:08:33 -0700 X-Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com [209.85.218.53]) by mx.groups.io with SMTP id smtpd.web10.281186.1697634512477650115 for ; Wed, 18 Oct 2023 06:08:32 -0700 X-Received: by mail-ej1-f53.google.com with SMTP id a640c23a62f3a-9be3b66f254so725258966b.3 for ; Wed, 18 Oct 2023 06:08:32 -0700 (PDT) X-Gm-Message-State: 7SxJnARGJ7ArBN1aoH8NitJhx7686176AA= X-Google-Smtp-Source: AGHT+IFbT53V3sus/1XCFZ4hToN7LwPg2LUjoHriq2dWQqRgbZfPCZBGMX8Z174B9nVyi2OdqIFSqU0NqUsQWx2oDw4= X-Received: by 2002:a17:907:a05:b0:9c4:b8c9:1bf3 with SMTP id bb5-20020a1709070a0500b009c4b8c91bf3mr4310925ejc.27.1697634509858; Wed, 18 Oct 2023 06:08:29 -0700 (PDT) MIME-Version: 1.0 References: <20231018103328.91093-1-kraxel@redhat.com> <0bc96936-0267-ef0c-a0bd-c36c5918af67@redhat.com> In-Reply-To: <0bc96936-0267-ef0c-a0bd-c36c5918af67@redhat.com> From: "Pedro Falcato" Date: Wed, 18 Oct 2023 14:08:16 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen To: devel@edk2.groups.io, lersek@redhat.com Cc: Gerd Hoffmann , Jordan Justen , Ard Biesheuvel , Oliver Steffen , Jiewen Yao , =?UTF-8?Q?Marvin_H=C3=A4user?= Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,pedro.falcato@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ULCFS7yb; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=gmail.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On Wed, Oct 18, 2023 at 1:20=E2=80=AFPM Laszlo Ersek wr= ote: > > On 10/18/23 13:33, Pedro Falcato wrote: > > On Wed, Oct 18, 2023 at 12:20=E2=80=AFPM Laszlo Ersek wrote: > >> > >> On 10/18/23 12:33, Gerd Hoffmann wrote: > >>> VirtiofsDxe throws an error in case the caller tries to open a file o= r > >>> directory using an handle with is not a directory, claiming that open= ing > >>> something relative to a file does not make sense. > >>> > >>> The claim is correct, but the code throws errors for both relative an= d > >>> absolute paths. Add a check to fix that. > >>> > >>> Signed-off-by: Gerd Hoffmann > >>> --- > >>> 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 referr= ed to > >>> // relative to a directory). > >>> // > >>> - if (!VirtioFsFile->IsDirectory) { > >>> + if (!VirtioFsFile->IsDirectory && FileName[0] !=3D '\\') { > >>> DEBUG (( > >>> DEBUG_ERROR, > >>> ("%a: Label=3D\"%s\" CanonicalPathname=3D\"%a\" FileName=3D\"%= s\": " > >> > >> It's nice to see this topic pop up on edk2-devel; apparently you start= ed > >> 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 > >>> 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 t= his > >>> strictly since the beginning, and a few months ago I reported USW= G Mantis > >>> ticket #2367 [1] too, for clearing up the related confusion in th= e 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 re= lax the > >>> check in VirtioFsSimpleFileOpen() a bit: if the pathname that's b= eing > >>> 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). Sometim= es you > >>> want to build upstream shim/grub binaries, but boot the same ISO > >>> otherwise. The fastest way for overriding the ESP for this purpos= e 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 wheth= er the > >>> just-built shim and grub are able to boot the rest of the ISO. > >>> > >>> [1] https://mantis.uefi.org/mantis/view.php?id=3D2367 > > > > 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... > Ok so I took some time to re-read the context and the spec. I think you're going about things the wrong way. The act of tolerating bad base "directories" in open calls is nothing new. For instance, in POSIX: int openat(int fd, const char *path, int oflag, ...); [...] The openat() function shall fail if: [...] [ENOTDIR] The path argument is not an absolute path and fd is a file descriptor associated with a non-directory file. so there's some track record in POSIX (in fact, openat is extra tolerant and can also take bad file descriptors if the path is absolute). Given that there may be software out there that does rely on this, it sure sounds like a better idea to be looser here, both in-driver and in-spec. What problem did GRUB have with this? GRUB freaking out with your patch sure does seem like it would freak out with any run-of-the-mill EnhancedFatDxe? (FWIW, I've just checked and EnhancedFatDxe does not seem to even check if the EFI_FILE is a directory, ever :)) --=20 Pedro -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109745): https://edk2.groups.io/g/devel/message/109745 Mute This Topic: https://groups.io/mt/102036263/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-