On Wed, Feb 08, 2023 at 08:20:17AM -0500, Michael S. Tsirkin wrote: > On Wed, Feb 08, 2023 at 01:23:48PM +0200, Dov Murik wrote: > > Hi Michael, > > > > On 08/02/2023 11:11, Michael S. Tsirkin wrote: > > > On Tue, Feb 07, 2023 at 07:33:09PM -0300, Jason A. Donenfeld wrote: > > >> On Tue, Feb 7, 2023 at 7:31 PM Michael S. Tsirkin <m...@redhat.com> > > >> wrote: > > >>> > > >>> On Tue, Feb 07, 2023 at 07:17:58PM -0300, Jason A. Donenfeld wrote: > > >>>> On Tue, Feb 07, 2023 at 04:45:19PM -0500, 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.lenda...@amd.com> > > >>>>>> Signed-off-by: Dov Murik <dovmu...@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] > > >>>>>> https://lore.kernel.org/qemu-devel/da39abab9785aea2a2e7652ed6403b6268aeb31f.ca...@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? > > >>>> > > >>>> I think that note is actually wrong. There prior was the sev_enabled() > > >>>> check elsewhere, which should have worked. I remember we originally had > > >>>> that problem with 7.1 and fixed it. So this is a new issue. I'll take > > >>>> care of it. > > >>>> > > >>>>> > > >>>>>> --- > > >>>>>> 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: 6661b8c7fe3f8b5687d2d90f7b4f3f23d70e3e8b > > >>>>> > > >>>>> I 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? > > >>>> > > >>>> Yes, highly object. If it's not here by default, it's completely > > >>>> useless > > >>>> from my perspective and I'll just stop working on this feature. There's > > >>>> no reason we can't make this work. It's turned out to have a lot of > > >>>> technical landmines, but that doesn't mean it's infeasible. I'll keep > > >>>> hammering away at it. > > >>>> > > >>>> Anyway, I'll send a v2 of this patch, and also address another thing > > >>>> left out of the previous fix. > > >>>> > > >>>> (And meanwhile, James and hpa@ seem to be having some discussion about > > >>>> introducing an even better mechanism; we'll see if that materializes.) > > >>>> > > >>>> Jason > > >>> > > >>> > > >>> OK I guess ... objections to a reverse flag disabling this? > > >>> Will at least allow a work-around for sev and friends ... > > >> > > >> I think we should generally try to make this work right as-is, without > > >> needing to introduce knobs. The SEV stuff seems really simple to fix. > > >> I'll have a 2 patch series for you in the next 20 minutes if all goes > > >> well. > > > > > > Absolutely. A knob can be a fallback though in the likely case > > > we missed something else. I'm inclined to > > > an on/off/auto knob which can either force it or let qemu > > > decide. Objections? > > > > > > > There's already a workaround for SEV: starting QEMU with > > '-machine pc-q35-7.1' (instead of '-machine q35'). > > The pc-q35-7.1 model sets > > > > pcmc->legacy_no_rng_seed = true; > > > > which prevents the modification of the cmdline (or modification of the > > kernel, in 7.2) -- and this allows the SEV kernel hashes to match. > > > > Of course this means that you don't get any other features of 7.2 or > > 8.0, if you need them. If we want to allow that, we'll need a special > > knob for turning off RNG seed. > > > > -Dov > > Right. Besides, this will also get you old bugs from 7.1 that > we kept around to stay compatible.
I think the 7.1 machine switch ought to be sufficient for folks while we work out whatever hypothetical bugs might be left after you merge the series I posted yesterday. That's why it was added in the first place. Jason