* Brijesh Singh (brijesh.si...@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 / dgilb...@redhat.com / Manchester, UK
