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 28592AC1956 for ; Fri, 20 Oct 2023 22:11:54 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=jtyMxbtBOUdQkwP4dhsc6mSPnxylW0Ify8Iw6dHhtyc=; 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=1697839913; v=1; b=HS1UZBnSPLYmiVPUXa6Dd0FsiCvVtppcsIK8a9RciWPWbXVL5em3d6vEl3DSiCAkv2poHj9x 382Jjemy9kW6tDBAuw5oHf2Ky0yMPIvt7nQ+C7cvGP4rML5ac5Hu92ROTp+wJysl7ROS6OyM8FH ispB1L/dAJqrB3MHl2iOebdo= X-Received: by 127.0.0.2 with SMTP id 9ZAKYY7687511x17J9Eeo7Oo; Fri, 20 Oct 2023 15:11:53 -0700 X-Received: from mail-vs1-f44.google.com (mail-vs1-f44.google.com [209.85.217.44]) by mx.groups.io with SMTP id smtpd.web11.67273.1697839913048908202 for ; Fri, 20 Oct 2023 15:11:53 -0700 X-Received: by mail-vs1-f44.google.com with SMTP id ada2fe7eead31-457e9088d7aso547596137.1 for ; Fri, 20 Oct 2023 15:11:52 -0700 (PDT) X-Gm-Message-State: Jhu9hl7FpBOXr3AtMaFCPsodx7686176AA= X-Google-Smtp-Source: AGHT+IHX1N/+qyRdYyfDsVkR+NhO9x/jUN0gRooGZKfo6ULe2QZUm9G7+6fHsGwsEe4VTy8Boksoi/ya2E1O7O2sGXo= X-Received: by 2002:a67:e154:0:b0:458:23f2:62e2 with SMTP id o20-20020a67e154000000b0045823f262e2mr3028781vsl.10.1697839912119; Fri, 20 Oct 2023 15:11:52 -0700 (PDT) MIME-Version: 1.0 References: <20231018172434.91280-1-lersek@redhat.com> In-Reply-To: From: "Pedro Falcato" Date: Fri, 20 Oct 2023 23:11:40 +0100 Message-ID: Subject: Re: [edk2-devel] [PATCH] OvmfPkg/VirtioFsDxe: tolerate opening an abs. pathname rel. to a reg. file To: Laszlo Ersek Cc: devel@edk2.groups.io, Ard Biesheuvel , Gerd Hoffmann , Jiewen Yao , Jordan Justen 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=HS1UZBnS; 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 Thu, Oct 19, 2023 at 3:16=E2=80=AFPM Laszlo Ersek wr= ote: > > On 10/19/23 15:50, Pedro Falcato wrote: > > On Wed, Oct 18, 2023 at 6:24=E2=80=AFPM Laszlo Ersek wrote: > >> > >> Referring to a file relative to a regular file makes no sense (or at l= east > >> 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 Man= tis > >> ticket #2367 [1] too, for clearing up the related confusion in the UEF= I > >> 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 t= he > >> check in VirtioFsSimpleFileOpen() a bit: if the pathname that's being > >> opened relative to a regular file is absolute, then the base file is g= oing > >> to be ignored anyway, so we can let the caller's bug slide. This happe= ns > >> 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 yo= u > >> 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 th= e > >> 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 > >> > >> 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 (o= r at least > >> // it cannot be implemented consistently with how a file is referre= d 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 Fi= leName 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= 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 > > 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*? --=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 (#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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-