Re: [PATCH] x86: Don't add RNG seed to Linux cmdline for SEV guests

[Michael S. Tsirkin]

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 <mst@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.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?
> >>>>
> >>>> 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.

-- 
MST