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

[Dov Murik]

On 08/02/2023 17:26, Tom Lendacky wrote:
> On 2/7/23 17:24, Jason A. Donenfeld wrote:
>> Hi Tom,
>>
>> On Tue, Feb 7, 2023 at 8:21 PM Tom Lendacky <thomas.lendacky@amd.com>
>> wrote:
>>>
>>> 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.
>>
>> Please let me know if this series fixes it:
>> 20230207224847.94429-1-Jason@zx2c4.com/">https://lore.kernel.org/all/20230207224847.94429-1-Jason@zx2c4.com/
> 
> I applied this series and it did fix the problem.
> 
> For SEV, there were two problems associated with the RNG seed support:
> 
> - The first is that it becomes part of the SEV LAUNCH measurement and
> therefore makes it impossible for the guest owner to be able to validate
> the measurement because the guest owner won't know the value of the RNG
> seed that was included in the LAUNCH measurement.
> 
> - The second problem is that the RNG is set and measured as part of the
> kernel-hashes support in x86_load_linux(), but it is changed via
> reset_rng_seed() before actually booting the first time. So the
> measurement taken in x86_load_linux() will never be the same when
> measured by, for example, OVMF.
> 
> So the addition of the !sev_enabled() check is definitely appropriate
> for the RNG seed support check for SEV.
> 
> However, is the change to the DTB check appropriate? Does the DTB vary /
> get updated before booting? If the DTB file is "constant" then the above
> two problems that face the RNG seed support shouldn't affect SEV. @Dov,
> does that make sense?
> 

Even if the DTB itself doesn't change and the Guest Owner could somehow add
it to the expected cmdline to calculate the hash, the current implementation
adds both the SetupData entry and the dtb itself to the cmdline.  The SetupData
entry contains pointers which may be harder to predict (even though currently 
I assume that .next=0 and the rest are known, so it should be possible (but 
ugly)).


But I now think there's another bug with the current patches: 


  /*
   * Add the NUL terminator, some padding for the microvm cmdline fiddling
   * hack, and then align to 16 bytes as a paranoia measure
   */
  cmdline_size = (strlen(machine->kernel_cmdline) + 1 +
                  VIRTIO_CMDLINE_TOTAL_MAX_LEN + 16) & ~15;
  /* Make a copy, since we might append arbitrary bytes to it later. */
  kernel_cmdline = g_strndup(machine->kernel_cmdline, cmdline_size);


This code at the beginning of x86_load_linux() will make the cmdline longer.
So the measurement of the cmdline will change (currently we expect it to be
the exactly value of the '-append' argument with a terminating '\0' byte).
This means that the SHA256 that QEMU and OVMF calculate is not the same as
what the Guest Owner will calculate for a given cmdline.

(this might be another argument for not pushing stuff at the end of the
cmdline and finding other dedicated pieces of memory for DTB / RNG_SEED / etc.).



> In any case, you'll need a version of the patch(es) that can be applied
> to the Qemu v7.2.0 tree to fix the regression.

Indeed.


-Dov