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

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

[Dov Murik]

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).

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] 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).
---
 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
-- 
2.25.1

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

[Jason A. Donenfeld]

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).
> 
> 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] 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).
> ---
>  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
> -- 
> 2.25.1
> 

Thanks for catching this. The same thing should also probably be set on
the dtb handling, and the commit message updated to reflect that this is
setup_data-specific, not just for the rng seed.

I'm concurrently working on a related bug fix in this same code. If you
want, I can roll a v2 of your patch (retaining your authorship) and
submit that together, if you want.

Jason

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

[Michael S. Tsirkin]

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] 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?

> ---
>  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?

> -- 
> 2.25.1

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

[Jason A. Donenfeld]

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] 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

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

[Michael S. Tsirkin]

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] 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 ...

-- 
MST

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

[Jason A. Donenfeld]

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] 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.

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

[Jason A. Donenfeld]

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] 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.

Done: https://lore.kernel.org/all/20230207224847.94429-1-Jason@zx2c4.com/

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

[Michael S. Tsirkin]

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] 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?

-- 
MST

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

[Dov Murik]

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] 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

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] 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

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

[Jason A. Donenfeld]

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] 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

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] 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

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

[Tom Lendacky]

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] 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: 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?
> 
>> -- 
>> 2.25.1
> 

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

[Jason A. Donenfeld]

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] 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:
https://lore.kernel.org/all/20230207224847.94429-1-Jason@zx2c4.com/

Jason

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

[Dov Murik]

On 08/02/2023 1: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] 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:
> https://lore.kernel.org/all/20230207224847.94429-1-Jason@zx2c4.com/>

I tested this series and it passes measured boot on SEV - OK.  I can
confirm that in non-SEV VM linux sees the SETUP_RNG_SEED; I didn't check
all the re-seeding scenarios.

-Dov

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

[Tom Lendacky]

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] 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:
> 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?

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.

Thanks,
Tom

> 
> Jason

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

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 09:26:28AM -0600, Tom Lendacky wrote:
> 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.

For 7.2 I think it's best to backport
 eac7a7791bb6 ("x86: don't let decompressed kernel image clobber setup_data")
(since that's also a bugfix) then this on top.

-- 
MST

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

[Jason A. Donenfeld]

On Wed, Feb 8, 2023 at 12:26 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> However, is the change to the DTB check appropriate?

Yes it is appropriate. The reason is that the first setup_data link is
already conditionalized on sev:

    /*
    * If we're starting an encrypted VM, it will be OVMF based, which uses the
    * efi stub for booting and doesn't require any values to be placed in the
    * kernel header.  We therefore don't update the header so the hash of the
    * kernel on the other side of the fw_cfg interface matches the hash of the
    * file the user passed in.
    */
   if (!sev_enabled() && first_setup_data) {
       SetupDataFixup *fixup = g_malloc(sizeof(*fixup));

       memcpy(setup, header, MIN(sizeof(header), setup_size));
       /* Offset 0x250 is a pointer to the first setup_data link. */

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]
>>>>> 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:
>> 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

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

[Jason A. Donenfeld]

On Wed, Feb 8, 2023 at 12:49 PM Dov Murik <dovmurik@linux.ibm.com> wrote:
> 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)).

No, setup_data isn't even hooked up under SEV. That part is skipped already.

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

[Dov Murik]

On 08/02/2023 17:49, Dov Murik wrote:
> 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.

Tomorrow I'll add debug code that prints the hashes table that QEMU
builds for SEV measured boot, and I'll check whether the hashes stay the
same (for given values of -kernel/-initrd/-append) across the versions
of QEMU.

-Dov

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

[Jason A. Donenfeld]

On Wed, Feb 8, 2023 at 12:49 PM Dov Murik <dovmurik@linux.ibm.com> wrote:
>   /*
>    * 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);

We could safely skip this part on !microvm, which I think might handle
the SEV case?

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

[Dov Murik]

On 08/02/2023 1:21, Tom Lendacky 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]
>>> 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.
> 

I see the same behaviour.


Also this was observed by the Confidential Containers project, which
uses kata-containers with QEMU to start SEV guest VMs; they downgraded
[1] from 7.2.0 to 7.1.0 until the issue is solved.

[1] https://github.com/kata-containers/kata-containers/pull/6191


They said they can backport a patch once it's upstream in qemu, but
since the RNG-seed code has changed a lot since 7.2.0, the patch that
we'll add in master will not be applicable as-is to 7.2.0.

I'm not sure if there's intention to release a 7.2.1 or who decides
about this.

-Dov

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

[Daniel P. Berrangé]

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] 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?
> 
> > ---
> >  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.

IMHO the main problem we have here is a lack of automated testing
coverage. There are too many subtle edge cases to rely on reviewers
spotting flaws in the code, we need automation.

Obviously our CI platforms don't have SEV hardware support[1], but we
still could have had an avocado test case in QEMU can be run manually
to validate things.

Also we should have avocado test case to cover SecureBoot with
-kernel, and that can be run in CI. And tests for the big kernel
scenario that this also broke with 7.2

With regards,
Daniel

[1] Anyone fancy adding SEV(ES|SNP) emulation to QEMU :-) Obviously
    would have to have separate certs/keys at the root of trust, but
    even with such a caveat it'd make life easier for developers and
    maintainers to not have to rely on real hardware all the time.
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Michael S. Tsirkin]

On Wed, Feb 08, 2023 at 09:35:21AM +0000, Daniel P. Berrangé 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] 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?
> > 
> > > ---
> > >  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.
> 
> IMHO the main problem we have here is a lack of automated testing
> coverage. There are too many subtle edge cases to rely on reviewers
> spotting flaws in the code, we need automation.
> 
> Obviously our CI platforms don't have SEV hardware support[1], but we
> still could have had an avocado test case in QEMU can be run manually
> to validate things.
> 
> Also we should have avocado test case to cover SecureBoot with
> -kernel, and that can be run in CI. And tests for the big kernel
> scenario that this also broke with 7.2

Could we not sign the empty image from bios test and use that with CI?
That signature can stay constant.

> With regards,
> Daniel
> 
> [1] Anyone fancy adding SEV(ES|SNP) emulation to QEMU :-) Obviously
>     would have to have separate certs/keys at the root of trust, but
>     even with such a caveat it'd make life easier for developers and
>     maintainers to not have to rely on real hardware all the time.


Yea this last one is what I thought too.



-- 
MST

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

[Daniel P. Berrangé]

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.

I presume you mean whether it impacts SecureBoot when using
-kernel with OVMF, but without SEV ?

IIRC, today OVMF ignores SecureBoot failures when using -kernel,
but we shouldn't make an assumption of that being the case on
the QEMU side.

> 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).
> 
> 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] 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).
> ---
>  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
> -- 
> 2.25.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dov Murik]

Hi Daniel,

On 08/02/2023 11:30, Daniel P. Berrangé 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.
> 
> I presume you mean whether it impacts SecureBoot when using
> -kernel with OVMF, but without SEV ?
> 
> IIRC, today OVMF ignores SecureBoot failures when using -kernel,
> but we shouldn't make an assumption of that being the case on
> the QEMU side.
> 

hmm, I'm not sure.  James mentioned something about Fedora attempting to
ship a unified signed kernel+cmdline+initrd package (and this RNG seed
addition to the cmdline will interfere), but maybe I'm confusing other
matters.

-Dov