From mboxrd@z Thu Jan 1 00:00:00 1970 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.902.1623168005373673528 for ; Tue, 08 Jun 2021 09:00:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UT2/Ftaa; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623168004; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=R3uP/KcCOpJQdio+kcTx69UhURpO5vydb73cpg9t+20=; b=UT2/FtaabXyeBNe6743V/0ofuOrbeuNRfVFnn0AQvm+cDTi1iFXExrUqtrVCxE3GMngP7d PF97M2USVJYbG/M2RJSS/VklNhup2yE4tEiOoFHtsehzUKz06jFKUoB7WQzPrTZ5ORCgBi JmlbbQ2qcLxBPGtCj1/VBhC+tuICSjA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-586-Typkju8FMwGDQMDpM7K1yQ-1; Tue, 08 Jun 2021 11:59:55 -0400 X-MC-Unique: Typkju8FMwGDQMDpM7K1yQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B5BDF1B2C984; Tue, 8 Jun 2021 15:59:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-124.ams2.redhat.com [10.36.112.124]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 885C25C1BB; Tue, 8 Jun 2021 15:59:50 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline To: Dov Murik , devel@edk2.groups.io, Ard Biesheuvel Cc: Tobin Feldman-Fitzthum , Tobin Feldman-Fitzthum , Jim Cadden , James Bottomley , Hubertus Franke , Jordan Justen , Ashish Kalra , Brijesh Singh , Erdem Aktas , Jiewen Yao , Min Xu , Tom Lendacky References: <20210525053116.1533673-1-dovmurik@linux.ibm.com> <5d8c598e-31de-7973-df51-e913bba54587@redhat.com> <3cead34f-a736-3a5d-4933-cebc085ca868@redhat.com> From: "Laszlo Ersek" Message-ID: <980736b6-2450-c695-98f5-84870c4ba3ee@redhat.com> Date: Tue, 8 Jun 2021 17:59:49 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/08/21 14:09, Dov Murik wrote: > On 08/06/2021 13:59, Laszlo Ersek wrote: >> On 06/08/21 11:57, Dov Murik wrote: >>> I started working on that, and managed to remove all QemuFwCfg* >>> calls in the main path of QemuLoadKernelImage (so far working on >>> X86QemuLoadImageLib.c). That works fine: I read the content of the >>> "cmdline" synthetic file, and I check the size of the synthetic >>> "initrd" file. I used Library/FileHandleLib.h; I hope that's fine. >> >> The lib class header says "Provides interface to EFI_FILE_HANDLE >> functionality", which is not too bad; but I don't immediately see >> what those APIs save us -- the APIs that I believe to be relevant to >> this use case all seem to be thin wrappers around EFI_FILE_PROTOCOL. >> (The only instance of FileHandleLib is >> "MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not >> necessarily a problem, it doesn't seem an obvious win (unless it >> saves you much complexity and/or code in a way that I'm missing). > > > Using FileHandleGetSize() saves some handling of a EFI_FILE_INFO > pointer and freeing it etc (for getting the size of "cmdline" and > "initrd"). But maybe it's better not to add another dependency. Hmm, no; you have convinced me. Please go ahead with FileHandleLib. Thank you for taking the time to talk this through with me. >>> Am I expected to change that legacy path as well? >>> Or is it in a "it's working don't touch" state? >>> If I modify this, how do I test it? >> >> The use case that you foresee for this feature is really important >> here. >> >> When you say that you don't reach QemuLoadLegacyImage(), that means >> your guest kernel is built with the UEFI stub. >> >> (1) If you can make the Linux UEFI stub a *required* part of the use >> case, then: >> >> (1a) switch the QemuLoadImageLib class resolution from >> "OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to >> "OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" >> in "OvmfPkg/AmdSev/AmdSevX64.dsc", >> >> (1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your >> modifications thus far should be easy to transplant to this lib >> instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no >> QemuLoadLegacyImage() in GenericQemuLoadImageLib. >> >> This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" >> does not offer Secure Boot support, so there's not going to be a case >> when gBS->LoadImage() rejects the UEFI stubbed Linux binary due to >> Secure Boot verification failing (EFI_SECURITY_VIOLATION, >> EFI_ACCESS_DENIED). >> >> >> (2) If you cannot make the Linux UEFI stub a required part of the use >> case, then X86QemuLoadImageLib needs to be modified indeed. >> >> (2a) Unfortunately, in this case we have to add a hack, because the >> synthetic filesystem only exposes the concatenated "setup data + >> kernel image" blob. The following would have to be preserved (as the >> only remaining QemuFwCfgLib access): >> >> QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize); >> SetupSize = (UINTN)QemuFwCfgRead32 (); >> >> (2b) and then the kernel blob from the synthetic fs would have to be >> split in two (= setup, kernel), within QemuLoadLegacyImage(). >> >> >> I'm sorry for missing this aspect previously! I really hope we can go >> with (1)! >> > > I'll check. > > But if we go with (1) -- do you (and Ard) prefer: > > (a) leave X86QemuLoadImageLib as it is in master; > > -or- > > (b) modify X86QemuLoadImageLib the "main" path to use the > QemuKernelLoaderFs (what I started doing) and leave the "legacy" path > with QemuFwCfg > > ? I prefer option (a), with the extension that we need to update the following file-top comment in the files under "OvmfPkg/Library/X86QemuLoadImageLib": X86 specific implementation of QemuLoadImageLib library class interface with support for loading mixed mode images and non-EFI stub images We should add a warning there that this library instance (a) depends on fw_cfg directly, and (b) is therefore unsuitable for blob verification purposes. > Or, in other words, is the refactoring to read the cmdline from > QemuKernelLoaderFs (across both QemuLoadImageLib implementations) > beneficial even if we don't add the verification "hooks"? My answer would be "no". IMO, the refactoring is only useful for unifying the blob verifier in a single layer (the synthetic fs). Without the blob verifier in the picture, I see nothing wrong with fetching different fw_cfg blobs in different modules. The kernel command line never needed to be available through the synthetic FS, for any particular consumer, so removing that synthetic file was fine. Now, we do have an internal consumer of sorts (the blob verifier). Given that we don't want to add a whole new extra layer for it, and we also don't want to scatter it over multiple modules, integrating it into the synthetic FS make sense, hopefully. I initially thought that the refactoring would benefit both QemuLoadImageLib instances, but you've shown that I missed the complications in the legacy path of X86QemuLoadImageLib. Namely, that the synthetic filesystem abstraction (= the fused setup data + kernel image blob) isn't a good match for the legacy path. Library instances are permitted to have different features and different limitations. (If we *really* wanted to do refactor the legacy path cleanly, we could further extend the QemuKernelLoaderFsDxe driver, to present -- under new filenames -- the setup data blob isolation, and the bare kernel image blob in isolation. But this would be a lot of wasted work, in practice, provided that your use case requires a UEFI stub on the guest kernel at all times (= the legacy path would never be taken). Also, let's not forget, if we exposed such *new* synthetic files, the blob verifier would have to verify those isolated blobs too (you'd have to provide hashes for them from the outside); otherwise the whole exercise would be moot, I think.) Thanks, Laszlo