| Message ID | 20230207084116.285787-1-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Don't add RNG seed to Linux cmdline for SEV guests | expand |
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
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
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
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 ...
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.
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/
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 >
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
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?
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
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.
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.
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
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
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
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
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.
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
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.
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
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.
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. */
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
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.
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
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?
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);
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(-) base-commit: 6661b8c7fe3f8b5687d2d90f7b4f3f23d70e3e8b