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 294F3941A9C for ; Wed, 18 Oct 2023 14:04:30 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=JzVpt9x9t/zfhr9TNAbOuNPYuDvZEJvUIi/9jTVZuBo=; 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=1697637868; v=1; b=gbh4g0lBuXRk+4m5FzK+14KCOiDrMeMRRaciNDta1KzdT6gmn9eGAVNFN5T/EO+QrOFd7YQK xUdKBnAIbTB6RfD/gbPLwvhtWx4+IcOI5fNUUqr/6NN1RMYSmKow4qGg9wAX31FP6qn9cdSyK26 rvmBdPrQ/CtohtbODVZnCaVk= X-Received: by 127.0.0.2 with SMTP id vUftYY7687511x4zwCjzDKaL; Wed, 18 Oct 2023 07:04:28 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.282422.1697637868107048402 for ; Wed, 18 Oct 2023 07:04:28 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-595-5QU2kaeEPjqymAERDhZrfA-1; Wed, 18 Oct 2023 10:04:17 -0400 X-MC-Unique: 5QU2kaeEPjqymAERDhZrfA-1 X-Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (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 E226F87A9F2; Wed, 18 Oct 2023 14:03:46 +0000 (UTC) X-Received: from [10.39.192.202] (unknown [10.39.192.202]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1041C908; Wed, 18 Oct 2023 14:03:44 +0000 (UTC) Message-ID: Date: Wed, 18 Oct 2023 16:03:43 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtioFsDxe: fix SimpleFileOpen To: Pedro Falcato , devel@edk2.groups.io Cc: Gerd Hoffmann , Jordan Justen , Ard Biesheuvel , Oliver Steffen , Jiewen Yao , =?UTF-8?Q?Marvin_H=c3=a4user?= References: <20231018103328.91093-1-kraxel@redhat.com> <0bc96936-0267-ef0c-a0bd-c36c5918af67@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.1 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: sip1ss0nfkuDoRch6sae6HMmx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=gbh4g0lB; 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 15:08, Pedro Falcato wrote: > On Wed, Oct 18, 2023 at 1:20 PM Laszlo Ersek wrote: >> >> On 10/18/23 13:33, Pedro Falcato wrote: >>> On Wed, Oct 18, 2023 at 12:20 PM 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 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/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 >>>>> 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... >> > > 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. I agree about being looser, as long as we label the change properly. And by properly, I mean "bug compat". I maintain that the UEFI spec is buggy -- it is inconsistent. The expression "the location of fh", where "fh" is a file handle, is ill-defined in the UEFI spec. My bug report for it (and my proposed fix for it) *restricts* the spec, turning the shim behavior into a clear-cut bug. (Note that the OpenEx protocol member spec already restricts "This" to be a directory! Which is actually the opposite of what you quote for openat() in POSIX.) We could further tweak the spec to add an exception for absolute pathnames, on top of my proposed restriction. But that's not the first step for fixing the 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? Wait, you are conflating things. There is a bug in the UEFI spec and (in parallel) in shim. The bug compat patch makes shim work. Therefore, in a shim+grub guest setup, grub can be *reached*. Then, grub cannot do anything with virtio-fs because it has no direct support for EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. It only has a (very questionable, IMO) block layer abstraction, on top of which it wants to use its own filesystem drivers (IIRC). I had looked into EFI_SIMPLE_FILE_SYSTEM_PROTOCOL enablement for grub, and my verdict was that grub's architecture makes that impossible to do *safely*. I don't want to rehash that investigation there; I did it half a year ago. > > (FWIW, I've just checked and EnhancedFatDxe does not seem to even > check if the EFI_FILE is a directory, ever :)) > EnhancedFatDxe is not scripture... For the record, here's the commit message for my (main) UEFI spec patch: EFI_FILE_PROTOCOL.Open(): restrict "This" to stand for a directory Make sure the Open() and OpenEx() languages consistently restrict "This" to be a directory file handle. Introduce the EFI_INVALID_PARAMETER return value to both functions, for when the restriction is not satisfied by the caller. Update the general description accordingly. [--*--] Consider the following pathname: \EFI\FOO Assume that we have an open file handle (an EFI_FILE_PROTOCOL) that refers to this filesystem object. Now further assume that we open the following pathname: BAR relative to the open file handle. What is the expectation for the full (absolute) pathname of the resultant object? Two choices: \EFI\FOO\BAR [1] \EFI\BAR [2] The first option makes sense if the original file handle, \EFI\FOO, stands for a directory. (That is, "FOO" is a subdirectory of "EFI".) And in that case, the second option is wrong. The second option makes sense, perhaps, if the original file handle, \EFI\FOO, stands for a regular file. (That is, "FOO" is a regular file in the "EFI" directory.) In that case, the first option is simply undefined -- \EFI\FOO identifies a regular file, so it has no child directory entries, such as \EFI\FOO\BAR. This means that when we attempt to open a pathname relative to a regular file, what we could mean in the best case would be a sibling, and not a child, of the last pathname component. Therefore, the UEFI spec language implies a child relationship for the relative pathname when the base file handle stands for an open directory, and *at best* a sibling relationship when the base file handle stands for an open regular file. This inconsistency is a bug. The language under EFI_FILE_PROTOCOL at : "With any given file handle, other files may be opened relative to this file's location" is *ill-defined* for regular files. Even if we attempt to retrofit the intended meaning from directories to regular files, the behavior will not be consistent -- see the *child* vs. *sibling* interpretations, respectively. To put differently, given "fh->Open()", the expression "the location of fh" is currently ill-defined in the UEFI spec. If "fh" is a directory, then its "location" is defined as "itself", in the usual sense of pathnames. But if "fh" stands for a regular file, then its "location" is *at best* defined as its *parent* directory. Because of this, filesystem objects relative to the "location of fh" are also ill-defined. It only becomes well-defined if we exclude regular files as base (source) handles. For opening filesystem objects with the EFI_FILE_PROTOCOL.Open() member function, the base handle must refer to a directory; it must not refer to a regular file. This implicit requirement is already correctly reflected in the OpenEx() language -- the Summary there explains "This" must be a directory. https://mantis.uefi.org/mantis/view.php?id=2367 Signed-off-by: Laszlo Ersek So I slightly wonder how EnhancedFatDxe resolves the *relative* pathname "BAR", when the base file handle stands for "\EFI\FOO", and "\EFI\FOO" is a regular file. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109754): https://edk2.groups.io/g/devel/message/109754 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/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-