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 76181740038 for ; Wed, 18 Oct 2023 11:20:32 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=sVuymverT75nOkEECafpxmQD7z2QyziC+ZA3S0e5d60=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1697628030; v=1; b=AERvxDYWdYS2opQrxiso9PNZ7q5MWZaVyO5hyp+3D+mFsCvi34Sxodn2BLk4q7kXl5s/Sdiz 36Kr9oAGZ+IrjI4sda2v2gyDAA3ZHLqpsnEMk477/o5uVpxtCeq9medV4rCqJiCVEtYztHZOIXi Zv3mfMVWjJhL0ZiS25xZ4fas= X-Received: by 127.0.0.2 with SMTP id hDZkYY7687511x40H2W0SLON; Wed, 18 Oct 2023 04:20:30 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.278737.1697628030212395690 for ; Wed, 18 Oct 2023 04:20:30 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-66-ACDWTc_dNkKYoihRZ4w5JA-1; Wed, 18 Oct 2023 07:20:26 -0400 X-MC-Unique: ACDWTc_dNkKYoihRZ4w5JA-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B274628AC1FC; Wed, 18 Oct 2023 11:20:25 +0000 (UTC) X-Received: from [10.39.192.202] (unknown [10.39.192.202]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 768952166B26; Wed, 18 Oct 2023 11:20:23 +0000 (UTC) Message-ID: Date: Wed, 18 Oct 2023 13:20:22 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen To: Gerd Hoffmann , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Oliver Steffen , Jiewen Yao References: <20231018103328.91093-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20231018103328.91093-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: CNEMdD5kP6NDUfsxs47RTVwBx7686176AA= Content-Language: en-US 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=AERvxDYW; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.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 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 > --- > OvmfPkg/VirtioFsDxe/SimpleFsOpen.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c b/OvmfPkg/VirtioFsDxe/Sim= pleFsOpen.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 t= o > // 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 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 > 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 Ma= ntis > ticket #2367 [1] too, for clearing up the related confusion in the UE= FI > spec. > > Unfortunately, the shim boot loader contains such a bug [2] [3]. I do= n'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 happ= ens > to make shim work. > > Why this matters: UEFI-bootable Linux installer ISOs tend to come wit= h > shim and grub in the embedded (ElTorito) FAT image (ESP). Sometimes y= ou > 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 fro= m > direct-booting a kernel (via fw_cfg); the point is to check whether t= he > just-built shim and grub are able to boot the rest of the ISO. > > [1] https://mantis.uefi.org/mantis/view.php?id=3D2367 > [2] https://bugzilla.redhat.com/show_bug.cgi?id=3D1966973 > [3] https://github.com/rhboot/shim/issues/382 > > Cc: Ard Biesheuvel > Cc: Gerd Hoffmann > Cc: Jiewen Yao > Cc: Jordan Justen > Signed-off-by: Laszlo Ersek > > Notes: > context:-U4 > > diff --git a/OvmfPkg/VirtioFsDxe/SimpleFsOpen.c b/OvmfPkg/VirtioFsDxe/Sim= pleFsOpen.c > index d479f76f5bc3..ec0521ac3703 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 a= t least > // it cannot be implemented consistently with how a file is referred t= o > - // 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 FileN= ame is > + // absolute, then VirtioFsAppendPath() below will disregard > + // VirtioFsFile->CanonicalPathname. > + // > + BugCompat =3D (FileName[0] =3D=3D L'\\'); > + > DEBUG (( > - DEBUG_ERROR, > + BugCompat ? DEBUG_WARN : DEBUG_ERROR, > ("%a: Label=3D\"%s\" CanonicalPathname=3D\"%a\" FileName=3D\"%s\":= " > "nonsensical request to open a file or directory relative to a re= gular " > "file\n"), > __FUNCTION__, > VirtioFs->Label, > VirtioFsFile->CanonicalPathname, > FileName > )); > - return EFI_INVALID_PARAMETER; > + if (!BugCompat) { > + return EFI_INVALID_PARAMETER; > + } > } > > // > // Allocate the new VIRTIO_FS_FILE object. Note that I'm adamant that this is a shim (and UEFI spec) bug, and that the current upstream code is right, *regardless* of whether the pathname to open starts with a backslash or not. The spec bug is reference [1] above, and the original incarnation of my shim bug report is reference [2]. Reference [3] is just the original RHBZ [2] having been migrated / copied to the upstream tracker. In other words, the patch is expressly a bug-compat patch. There are two reasons why I never posted the patch: (1) The (non-)treatment I received from the shim maintainers in ticket discouraged me from doing anything with, or for, shim. (2) With this modification in place, shim is happy, but grub isn't. When I realized that, I looked relatively deeply into making grub work on top of virtio-fs as well -- and my findings were horrendous. I wrote up my findings in a private email to some colleagues; you were among the recipients. The Message-Id of that email is <244b4c0f-8c79-7cd6-193e-54046ecf323c@redhat.com>, and the date is "4/19/23, 15:18". My main statement in that email was that grub2 was *architecturally incompatible* with UEFI, and I added: > What I mean by architecturally incompatible: grub2 is designed from a > perspective where it thinks it is an operating system; in other words, > that it *owns* the computer. The problem is that UEFI thinks the exact > same thing of itself, and of course the two conflict. I elaborated a great detail on that, providing various examples, in particular in relation to how grub used EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. Those findings were what made me deem my shim bugcompat patch futile, after all -- I didn't post the patch because grub was unsalvageable anyway, so I didn't see the point. If you have a use case where you rely on shim but *not* on Grub (UKIs?), then I'm OK relaxing the strictness of VirtioFsDxe. In that case, I'd prefer upstreaming my above patch, from April, rather than taking yours. What do you think about that? --*-- Here's a further (independent) caveat: if you are using VirtioFsDxe with the rust language virtiofsd, then you might experience hangs in VirtioFsInit. For fixing that, you need the following *qemu* patch set: [PATCH v3 0/7] vhost-user: call VHOST_USER_SET_VRING_ENABLE synchronously https://patchew.org/QEMU/20231002203221.17241-1-lersek@redhat.com/ (This patch set has been on qemu-devel for nearly 2 months now, counting from v1; I'm going to ping MST again. It's been ready for merging for weeks now!) Thanks! Laszlo -=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 (#109729): https://edk2.groups.io/g/devel/message/109729 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-