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

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 02:30:30PM +0100, Jason A. Donenfeld wrote:
> 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 <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.
> 
> 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

Not really, it was added because 7.1 did not have it and we wanted
to be compatible. I just suspect we'll find more bugs and again
after a release once people use it in the field.

-- 
MST