On 11/20/23 17:50, Gerd Hoffmann wrote: > On Mon, Nov 20, 2023 at 12:53:45PM +0100, Alexander Graf wrote: >> Hey Gerd! >> >> On 15.11.23 16:12, Gerd Hoffmann wrote: >>> This patch adds a virtual device to qemu which the uefi firmware can use >>> to store variables. This moves the UEFI variable management from >>> privileged guest code (managing vars in pflash) to the host. Main >>> advantage is that the need to have privilege separation in the guest >>> goes away. >>> >>> On x86 privileged guest code runs in SMM. It's supported by kvm, but >>> not liked much by various stakeholders in cloud space due to the >>> complexity SMM emulation brings. >>> >>> On arm privileged guest code runs in el3 (aka secure world). This is >>> not supported by kvm, which is unlikely to change anytime soon given >>> that even el2 support (nested virt) is being worked on for years and is >>> not yet in mainline. >>> >>> The design idea is to reuse the request serialization protocol edk2 uses >>> for communication between SMM and non-SMM code, so large chunks of the >>> edk2 variable driver stack can be used unmodified. Only the driver >>> which traps into SMM mode must be replaced by a driver which talks to >>> qemu instead. >> >> >> I'm not sure I like the split :). If we cut things off at the SMM >> communication layer, we still have a lot of code inside the Runtime Services >> (RTS) code that is edk2 specific which means we're tying ourselves tightly >> to the edk2 data format. > > Which data format? > > Request serialization format? Yes. I can't see what is wrong with > that.
... I can't even see what's wrong with *that*. Realistically / practically, I think only edk2 has been considered as guest UEFI firmware for QEMU/KVM virtual machines, as far as upstream projects go. It's certainly edk2 that's bundled with QEMU. My understanding is that firmware is just considered a part of the virtualization platform, so teaching edk2 specifics to QEMU doesn't seem wrong. (As long as we have the personpower to maintain interoperability.) > We need one anyway, and I don't see why inventing a new one is > any better than the one we already have in edk2. > > Variable storage format? Nope, that is not the case. The variable > driver supports a cache, which I think is a read-only mapping of the > variable store, so using that might imply we have to use edk2 storage > format. Didn't check in detail through. The cache is optional, can be > disabled at compile time using PcdEnableVariableRuntimeCache=FALSE, and > I intentionally do not use the cache feature, exactly to avoid unwanted > constrains to the host side implementation. > >> It also means we can not easily expose UEFI >> variables that QEMU owns, > > Qemu owning variables should be no problem. Adding monitor commands to > read/write UEFI variables should be possible too. This patch set is actually an improvement towards QEMU-owned variables. Right now, all variables are internal to the guest; QEMU only has a pflash-level view. > >> which can come in very handy when implementing MOR >> - another feature that depends on SMM today. > > Have a pointer for me? Google finds me > https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-guard-requirements, > which describes the variable behavior (which I think should be no > problem to implement), but doesn't say a word about what exactly gets > locked when enabled ... See: TCG PC Client Platform Reset Attack Mitigation Specification My copy is Family “2.0” Version 1.10 Revision 17 January 21, 2019 Published You should find it somewhere in the download area of <https://trustedcomputinggroup.org>. E.g.... <https://www.trustedcomputinggroup.org/wp-content/uploads/Platform-Reset-Attack-Mitigation-Specification.pdf> In the past we've had a bunch of discussions / patches around this. Some examples: - [edk2] multiple levels of support for MOR / MORLock http://mid.mail-archive.com/039cf353-80fb-9f20-6ad2-f52517ab4de7@redhat.com - https://bugzilla.tianocore.org/show_bug.cgi?id=727 (see edk2 commit range listed there, too) - commit 704b71d7e11f115a3b5b03471d6420a7a70f1585 - commit d20ae95a13e851d56c6618108b18c93526505ca2 - https://bugzilla.redhat.com/show_bug.cgi?id=1854212 - https://bugzilla.redhat.com/show_bug.cgi?id=1498159 > >> In EC2, we are simply serializing all variable RTS calls to the hypervisor, > > The edk2 code effectively does the same (with > PcdEnableVariableRuntimeCache=FALSE). > >> similar to the Xen implementation >> (https://www.youtube.com/watch?v=jiR8khaECEk). > > Is the Xen implementation upstream? Can't see a xen variable driver in > OvmfPkg. The video is from 2019. What is the state of this? Not sure about the current state, but when that presentation came out, we discussed it briefly internally. I don't have time to review that old discussion now for the sake of potentially publishing it inside this discussion, but for interested Red Hatters, I can provide a pointer: [virt-devel] Securing Secure Boot on Xen | FOSDEM'19 Date: Fri, 8 Feb 2019 20:46:46 +0100 msgid: <37382312-986c-4ce4-1ba3-942697738...@redhat.com> > >> The edk2 side code we have built is here: >> https://github.com/aws/uefi/tree/main/edk2-stable202211 (see anything with >> VarStore in the name). Well... why was this never upstreamed to edk2? (Side question, of course.) > > Ok, so it's just the variables. The edk2 variant also sends variable > policy requests (see Edk2VariablePolicyProtocol, > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/VariablePolicyLib/ReadMe.md). > And it can easily be extended should the need arise in the future (all > requests are tagged with a protocol/event guid). > >> Given that we deal with untrusted input, I would strongly prefer if we could >> move it to a Rust implementation instead though. > > Valid point. Agree that the point is valid. (And this was one of my concerns, if not my main concern, in the above internal discussion, about Xen varstored.) Especially due to the heavy crypto that the final version is supposed to have. Even during the present patch review, while going through only the headers thus far, I've already said at least twice that we're going to have to be super careful about integer overflows and buffer overflows. Any such problem is no longer a guest<->guest privilege boundary breach but a guest<->host one. Not sure if the suggested remedy ("write it in Rust") is practical. > > I've started in C because I have next no to experience with rust (yet), > so getting a test-able / demo-able implementation done was much easier > for me. Also I think we don't have any rust infrastructure in qemu > (yet?). Being able to use the qapi / qobject support to read/write the > variable store in json format is nice too. > > But I'm open to discuss other options. A selfish aspect: given that I've been reviewing this set, should I consider it a proof of concept / prototype, or something we might want to build upon, i.e., should I assume we'd put these foundations into production at some point? I've been reviewing the series with the latter in mind, but if that's not correct, I should probably adjust my pedantry knob. > >> We started a Rust >> reimplementation of it that interface that can build as a library with C >> bindings which QEMU could then link against: >> >> https://github.com/Mimoja/rs-uefi-varstore/tree/for_main >> >> The code never went beyond the initial stages, > > Hmm. Why not? And I'm a bit annoyed that you had to write ~2500 lines of QEMU code (not counting edk2) for qemu-devel to learn about these initiatives. :/ > >> but if we're pulling the variable store to QEMU, this would be the >> best path forward IMHO. > > Ok, so you are trying to sell me a prototype as solution? > /me looks a bit skeptical ... at least with virtiofsd, we had gone with a C impl first, and only then with a Rust impl... Laszlo
