|
From: | Tom Lendacky |
Subject: | Re: [PATCH] x86: Don't add RNG seed to Linux cmdline for SEV guests |
Date: | Tue, 7 Feb 2023 17:21:33 -0600 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1 |
On 2/7/23 15:45, Michael S. Tsirkin wrote:
On Tue, Feb 07, 2023 at 08:41:16AM +0000, Dov Murik wrote:Recent feature to supply RNG seed to the guest kernel modifies the kernel command-line by adding extra data at its end; this breaks measured boot with SEV and OVMF, and possibly signed boot. Specifically SEV doesn't miss this feature because it uses UEFI/OVMF which has its own way of getting random seed (not to mention that getting the random seed from the untrusted host breaks the confidential computing trust model).Nope - getting a random seed from an untrusted source should not break anything assuming you also have some other randomness source. If you don't then you have other problems.Disable the RNG seed feature in SEV guests. Fixes: eac7a7791bb6 ("x86: don't let decompressed kernel image clobber setup_data") Reported-by: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> --- There might be a need for a wider change to the ways setup_data entries are handled in x86_load_linux(); here I just try to restore the situation for SEV guests prior to the addition of the SETUP_RNG_SEED entry. Recent discussions on other (safer?) ways to pass this setup_data entry: [1] da39abab9785aea2a2e7652ed6403b6268aeb31f.camel@linux.ibm.com/">https://lore.kernel.org/qemu-devel/da39abab9785aea2a2e7652ed6403b6268aeb31f.camel@linux.ibm.com/ Note that in qemu 7.2.0 this is broken as well -- there the SETUP_RNG_SEED entry is appended to the Linux kernel data (and therefore modifies and breaks the measurement of the kernel in SEV measured boot). A similar fix will be needed there (but I fear this patch cannot be applied as-is).So it's not a regression, is it?
SEV kernel hash comparison succeeded with Qemu v7.1.0, but fails with v7.2.0, so I think that is a regression.
Thanks, Tom
--- hw/i386/x86.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/i386/x86.c b/hw/i386/x86.c index eaff4227bd..e65a83f8df 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -1103,7 +1103,7 @@ void x86_load_linux(X86MachineState *x86ms, load_image_size(dtb_filename, setup_data->data, dtb_size); }- if (!legacy_no_rng_seed && protocol >= 0x209) {+ if (!legacy_no_rng_seed && protocol >= 0x209 && !sev_enabled()) { setup_data_offset = cmdline_size; cmdline_size += sizeof(SetupData) + RNG_SEED_LENGTH; kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size); base-commit: 6661b8c7fe3f8b5687d2d90f7b4f3f23d70e3e8bI am beginning to think we have been hasty here. no rng seed should have been then default and requested with a flag. Then we'd avoid all this heartburn - and SEV might not be the only workload broken. Maybe not too late. Jason - objections?-- 2.25.1
[Prev in Thread] | Current Thread | [Next in Thread] |