Re: [PATCH 0/3] SEV: fixes for -kernel launch with incompatible OVMF

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> 
> 
> On 11/2/21 8:22 AM, Dov Murik wrote:
> > 
> > 
> > On 02/11/2021 12:52, Brijesh Singh wrote:
> > > Hi Dov,
> > > 
> > > Overall the patch looks good, only question I have is that now we are
> > > enforce qemu to hash the kernel, initrd and cmdline unconditionally for
> > > any of the SEV guest launches. This requires anyone wanting to
> > > calculating the expected measurement need to account for it. Should we
> > > make the hash page build optional ?
> > > 
> > 
> > The problem with adding a -enable-add-kernel-hashes QEMU option (or
> > suboption) is yet another complexity for the user.  I'd also argue that
> > adding these hashes can lead to a more secure VM boot process, so it
> > makes sense for it to be the default (and maybe introduce a
> > -allow-insecure-unmeasured-kernel-via-fw-cfg option to prevent the
> > measurement from changing due to addition of hashes?).
> > 
> > Maybe, on the other hand, OVMF should "report" whether it supports
> > hashes verification. If it does, it should have the GUID in the table
> > (near the reset vector), like the current OvmfPkg/AmdSev edk2 build. If
> > it doesn't support that, then the entry should not appear at all, and
> > then QEMU won't add the hashes (with patch 1 from this series).  This
> > means that in edk2 we need to remove the SEV Hash Table block from the
> > ResetVectorVtf0.asm for OvmfPkg, but include it in the AmdSev build.
> > 
> 
> By leaving it ON is conveying a wrong message to the user. The library used
> for verifying the hash is a NULL library for all the builds of Ovmf except
> the AmdSev package. In the NULL library case, OVMF does not perform any
> checks and hash table is useless. I will raise this on concern on your Ovmf
> patch series.
> 
> IMHO, if you want to turn it ON by default then make sure all the OVMF
> package builds supports validating the hash.
> 
> 
> > But the problem with this approach is that it prevents the future
> > unification of AmdSev and OvmfPkg, which is a possibility we discussed
> > (at least with Dave Gilbert), though not sure it's a good/feasible goal.
> > 
> > 
> 
> This is my exact concern, we are auto enabling the features in Qemu that is
> supported by AmdSev package only.

I'm confused; wouldn't the trick be to only define the GUIDs for the
builds that support the validation?

Dave

> 
> > 
> > > I am thinking this more for the SEV-SNP guest. As you may be aware that
> > > with SEV-SNP the attestation is performed by the guest, and its possible
> > > for the launch flow to pass 512-bits of host_data that gets included in
> > > the report. If a user wants to do the hash'e checks for the SNP then
> > > they can pass a hash of kernel, initrd and cmdline through a
> > > launch_finish.ID_BLOCK.host_data and does not require a special hash
> > > page. This it will simplify the expected hash calculation.
> > 
> > That is a new measured boot "protocol" that we can discuss, and see
> > whether it's better/easier than the existing one at hand that works on
> > SEV and SEV-ES.
> > 
> > What I don't understand in your suggestion is who performs a SHA256 of
> > the fw_cfg blobs (kernel/initrd/cmdline) so they can later be verified
> > (though ideally earlier is better).  Can you describe the details
> > (step-by-step) of an SNP VM boot with -kernel/-initrd/-append and how
> > the measurement/attestation is performed?
> > 
> > 
> 
> There are a multiple ways on how you can do a measured boot with the SNP.
> 
> 1) VMPL0 (SVSM) can provide a complete vTPM (see the MSFT proposal on SNP
> mailing list).
> 
> 2) Use your existing hashing approach with some changes to provide a bit
> more flexibility.
> 
> 3) Use your existing hashing approach but zero out the hash page when
> -kernel is not used.
> 
> Let me expand #2.
> 
> While launching the SNP guest, a guest owner can provide a ID block that KVM
> will pass to the PSP during the guest launch flow. In the ID block there is
> a field called "host_data". A guest owner can do a hash of
> kernel/initrd/cmdline and include it in the "host_data" field. During the
> hash verification, the OVMF can call the SNP_GET_REPORT. The PSP will
> includes the "host_data" passed in the launch process in the report and OVMF
> can use it for the verification. Unlike the current implementation, this
> enables a guest owner to provides the hash without requiring any changes in
> the Qemu and thus affecting the measurement.
> 
> One thing to note that both #2 and #3 requires ovmf to connect to guest
> owner to validate the report before using the "host_data" or "hash page".
> 
> 
> thanks
> 
> > 
> > > Adding a
> > > special page requires a validation of that page. All the prevalidated
> > > page need to be excluded by guest BIOS page validation flow to avoid the
> > > double validation. The hash page is populated only when we pass -kernel
> > > and it will be tricky to communicate this information to the guest BIOS
> > > so that it can skip the validation.
> > 
> > So that again comes back to the earlier question of whether we should
> > always fill the hashes page or only sometimes, and how can OVMF tell.
> > 
> > How about: QEMU always prevalidates this page (either fills it with
> > zeros or with the hashes table), and the BIOS always excludes it?
> > 
> > -Dov
> > 
> > 
> > > 
> > > Thoughts ?
> > > 
> > > thanks
> > > 
> > > On 11/1/21 5:21 AM, Dov Murik wrote:
> > > > Tom Lendacky and Brijesh Singh reported two issues with launching SEV
> > > > guests with the -kernel QEMU option when an old [1] or wrongly configured [2]
> > > > OVMF images are used.
> > > > 
> > > > The fixes in patches 1 and 2 allow such guests to boot by skipping the
> > > > kernel/initrd/cmdline hashes addition to the initial guest memory (and
> > > > warning the user).
> > > > 
> > > > Patch 3 is a refactoring of parts of the same function
> > > > sev_add_kernel_loader_hashes() to calculate all padding sizes at
> > > > compile-time.  This patch is not required to fix the issues above, but
> > > > is suggested as an improvement (no functional change intended).
> > > > 
> > > > Note that launch measurement security is not harmed by these fixes: a
> > > > Guest Owner that wants to use measured Linux boot with -kernel, must use
> > > > (and measure) an OVMF image that designates a proper hashes table area,
> > > > and that verifies those hashes when loading the binaries from QEMU via
> > > > fw_cfg.
> > > > 
> > > > The old OVMFs which don't publish the hashes table GUID or don't reserve
> > > > a valid area for it in MEMFD cannot support these hashes verification in
> > > > any case (for measured boot with -kernel).
> > > > 
> > > > 
> > > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F3b9d10d9-5d9c-da52-f18c-cd93c1931706%40amd.com%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cffa0a5981860476c3bcc08d99e03d3d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637714561554218974%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=591wZvEzQQQ6JBjLDhGnvEM8fxX6iky9yxlWn2pifjI%3D&reserved=0
> > > > [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F001dd81a-282d-c307-a657-e228480d4af3%40amd.com%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cffa0a5981860476c3bcc08d99e03d3d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637714561554218974%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ihwNJjetXq5I0WaLjEFzhtrKMbj%2FaFmOmn1xYlLowjg%3D&reserved=0
> > > > 
> > > > Dov Murik (3):
> > > >    sev/i386: Allow launching with -kernel if no OVMF hashes table found
> > > >    sev/i386: Warn if using -kernel with invalid OVMF hashes table area
> > > >    sev/i386: Perform padding calculations at compile-time
> > > > 
> > > >   target/i386/sev.c | 34 +++++++++++++++++++++++-----------
> > > >   1 file changed, 23 insertions(+), 11 deletions(-)
> > > > 
> > > > 
> > > > base-commit: af531756d25541a1b3b3d9a14e72e7fedd941a2e
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK