On 11/2/21 8:22 AM, Dov Murik wrote:
On 02/11/2021 12:52, Brijesh Singh wrote:
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
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.