From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web09.973.1623168064932521204 for ; Tue, 08 Jun 2021 09:01:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ImkSuEVq; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623168064; 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=nIxMHpJK/FQ1mqfF1w2ZPiif+1+dkUeAdZGQyujUHAs=; b=ImkSuEVqC+xztMiII5ptAWC5tEQzQgUvpdbf+B2eQdo7EhjOfcTOgZxdzJi5wEHw3Z/ayZ GUrj6aFZgxUgoi/fWQkKgoVbLHeEKrfkBBmc5k6IAG/iFOor/lnVATL1QAaB8IwXgdybpa HnoK8VHcn/OHtnFWlhG37sl8a3GxOIo= 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-105-MuYbghlAOxe8Z-VX32M_Yw-1; Tue, 08 Jun 2021 12:01:02 -0400 X-MC-Unique: MuYbghlAOxe8Z-VX32M_Yw-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 7D1D81B2C981; Tue, 8 Jun 2021 16:01:00 +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 2ADC95C1BB; Tue, 8 Jun 2021 16:00:56 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline To: Ard Biesheuvel Cc: Dov Murik , edk2-devel-groups-io , Ard Biesheuvel , 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: Date: Tue, 8 Jun 2021 18:00:55 +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:49, Ard Biesheuvel wrote: > On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek wrote: >> >> Ard, >> >> do you have any comments please, on the topic at the bottom? >> >> My comments follow: >> >> On 06/08/21 11:57, Dov Murik wrote: >>> >>> >>> On 04/06/2021 14:26, Laszlo Ersek wrote: >>>> On 06/04/21 12:30, Dov Murik wrote: >>>> >>> >>> ... >>> >>>>> >>>>>> [Ard, please see this one question:] >>>>>> >>>>>> - A major complication for hashing all three of: kernel, initrd, >>>>>> cmdline, is that the *fetching* of this triplet is split between >>>>>> two places. (Well, it is split between *three* places in fact, but >>>>>> I'm going to ignore LinuxInitrdDynamicShellCommand for now, because >>>>>> the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.) >>>>>> >>>>>> The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but >>>>>> the command line is fetched in (both) QemuLoadImageLib instances. >>>>>> This requires that all these modules be littered with hashing as >>>>>> well, which I find *really bad*. Even if we factor out the actual >>>>>> logic, I strongly dislike having *just hooks* for hashing in >>>>>> multiple modules. >>>>>> >>>>>> Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe: >>>>>> don't expose kernel command line", 2020-03-05). If we first >>>>>> >>>>>> (a) reverted that commit, and >>>>>> >>>>>> (b) modified *both* QemuLoadImageLib instances, to load the kernel >>>>>> command line from the *synthetic filesystem* (rather than directly >>>>>> from fw_cfg), >>>>>> >>>>>> then we could centralize the hashing to just QemuKernelLoaderFsDxe. >>>>>> >>>>>> Ard -- what's your thought on this? >>>>>> >>>>> >>>>> I understand there's agreement here, and that both this suggested >>>>> change (use the synthetic filesystem) and my patch series (add hash >>>>> verification) touch the same code areas. How do you envision this >>>>> process in the mailing list? Seperate patch serieses with >>>>> dependency? One long patch series with both changes? What goes >>>>> first? >>>> >>>> Good point. I do have a kind of patch order laid out in my mind, but >>>> I didn't think of whether we should have the patches in one patch >>>> series, or in two "waves". >>>> >>>> OK, let's go with two patch sets. >>>> >>>> In the first set, we should just focus on the above steps (a) and >>>> (b). Step (a) shouldn't be too hard. In step (b), you'd modify both >>>> QemuLoadImageLib instances (two separate patches), replacing the >>>> QemuFwCfgLib APIs for fetching the cmdline with >>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs. >>>> >>>> Speaking from memory, the synthetic filesystem has a unique device >>>> path, so the first step would be calling gBS->LocateDevicePath(), for >>>> finding SimpleFs on the unique device path. Once you have the >>>> SimpleFs interface, you can call OpenVolume, then open the "cmdline" >>>> file using the EFI_FILE_PROTOCOL output by OpenVolume. >>>> >>>> Once we merge this series (basically just three patches), there is no >>>> QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I >>>> reckon. >>> >>> 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). >> >> In OVMF, the following executables use UefiFileHandleLib at the moment: >> >> - MdeModulePkg/Universal/BdsDxe/BdsDxe.inf >> - ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf >> - ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf >> - OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf >> - ShellPkg/Application/Shell/Shell.inf >> >> The last four are shell-related, so "prior art" is really just BdsDxe... >> >>> However, there's another path (which I don't reach with my test >>> setup), which is the call to QemuLoadLegacyImage, which has a lot of >>> calls to QemuFwCfg* in its body. >>> >>> 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)! >> > > The legacy x86 loader should only be kept if there is a strong need to > do so. Keeping it around for some theoretical compatibility concern is > simply not worth it, IMHO. > > Having two boot paths to reason about in terms of security is not the > only problem: another concern is that the legacy fallback path is > strictly x86, and is tightly coupled with the kernel's struct > boot_params, which is only documented by the .h file that happens to > declare it (and some outdated prose under Documentation/ perhaps) > > Also, the EFI stub does some non-trivial work at boot, and having this > uniform between architectures is an important goal, especially for > non-x86 folks like me. We have introduced an initrd loader mechanism > that is fully arch agnostic, and there are patches on the list to move > the measurement of the initrd into the EFI stub if it was loaded using > this mechanism. > > In summary, please stick with the generic loader unless you *really* can't. > Awesome, thank you! Laszlo