Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, 2023-12-06 at 11:42 -0600, Michael Roth wrote:
> On Wed, Dec 06, 2023 at 07:20:14PM +0200, Maxim Levitsky wrote:
> > On Tue, 2023-12-05 at 16:28 -0600, Michael Roth wrote:
> > > Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> > > added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
> > > exposed a long-running bug in current KVM support for SEV-ES where the
> > > kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
> > > kernel, in which case EFER write traps would result in KVM eventually
> > > seeing MSR_EFER_LMA get set and recording it in such a way that it would
> > > be subsequently visible when accessing it via KVM_GET_SREGS/etc.
> > >
> > > However, guests kernels currently rely on MSR_EFER_LMA getting set
> > > automatically when MSR_EFER_LME is set and paging is enabled via
> > > CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
> > > MSR_EFER_LMA even though it is set internally, and when QEMU
> > > subsequently tries to pass this EFER value back to KVM via
> > > KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
> > > which is now considered fatal due to the aforementioned QEMU commit.
> > >
> > > This can be addressed by inferring the MSR_EFER_LMA bit being set when
> > > paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
> > > the expected bits are all present in subsequent handling on the host
> > > side.
> > >
> > > Ultimately, this handling will be implemented in the host kernel, but to
> > > avoid breaking QEMU's SEV-ES support when using older host kernels, the
> > > same handling can be done in QEMU just after fetching the register
> > > values via KVM_GET_SREGS*. Implement that here.
> > >
> > > Cc: Paolo Bonzini
> > > Cc: Marcelo Tosatti
> > > Cc: Tom Lendacky
> > > Cc: Akihiko Odaki
> > > Cc: k...@vger.kernel.org
> > > Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> > > Signed-off-by: Michael Roth
> > > ---
> > > v2:
> > > - Add handling for KVM_GET_SREGS, not just KVM_GET_SREGS2
> > >
> > > target/i386/kvm/kvm.c | 14 ++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > > index 11b8177eff..8721c1bf8f 100644
> > > --- a/target/i386/kvm/kvm.c
> > > +++ b/target/i386/kvm/kvm.c
> > > @@ -3610,6 +3610,7 @@ static int kvm_get_sregs(X86CPU *cpu)
> > > {
> > > CPUX86State *env = >env;
> > > struct kvm_sregs sregs;
> > > +target_ulong cr0_old;
> > > int ret;
> > >
> > > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS, );
> > > @@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
> > > env->gdt.limit = sregs.gdt.limit;
> > > env->gdt.base = sregs.gdt.base;
> > >
> > > +cr0_old = env->cr[0];
> > > env->cr[0] = sregs.cr0;
> > > env->cr[2] = sregs.cr2;
> > > env->cr[3] = sregs.cr3;
> > > env->cr[4] = sregs.cr4;
> > >
> > > env->efer = sregs.efer;
> > > +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> > > +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> > > +env->efer |= MSR_EFER_LMA;
> > > +}
> > > +}
> >
> > I think that we should not check that CR0_PG has changed, and just blindly
> > assume
> > that if EFER.LME is set and CR0.PG is set, then EFER.LMA must be set as
> > defined in x86 spec.
> >
> > Otherwise, suppose qemu calls kvm_get_sregs twice: First time it will work,
> > but second time CR0.PG will match one that is stored in the env, and thus
> > the workaround
> > will not be executed, and instead we will revert back to wrong EFER value
> > reported by the kernel.
> >
> > How about something like that:
> >
> >
> > if (sev_es_enabled() && env->efer & MSR_EFER_LME && env->cr[0] &
> > CR0_PG_MASK) {
> > /*
> > * Workaround KVM bug, because of which KVM might not be aware of
> > the
> > * fact that EFER.LMA was toggled by the hardware
> > */
> > env->efer |= MSR_EFER_LMA;
> > }
>
> Hi Maxim,
>
> I'd already sent a v3 based on a similar suggestion from Paolo:
>
> https://lists.gnu.org/archive/html/qemu-devel/2023-12/msg00751.html
>
> Does that one look okay to you?
Yep, thanks!
Best regards,
Maxim Levitsky
>
> Thanks,
>
> Mike
>
> >
> > Best regards,
> > Maxim Levitsky
> >
> > >
> > > /* changes to apic base and cr8/tpr are read back via
> > > kvm_arch_post_run */
> > > x86_update_hflags(env);
> > > @@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
> > > {
> > > CPUX86State *env = >env;
> > > struct kvm_sregs2 sregs;
> > > +target_ulong cr0_old;
> > > int i, ret;
> > >
> > > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
> > > @@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
> > > env->gdt.limit = sregs.gdt.limit;
> > > env->gdt.base = sregs.gdt.base;
> > >
> > >
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, Dec 06, 2023 at 07:20:14PM +0200, Maxim Levitsky wrote:
> On Tue, 2023-12-05 at 16:28 -0600, Michael Roth wrote:
> > Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> > added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
> > exposed a long-running bug in current KVM support for SEV-ES where the
> > kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
> > kernel, in which case EFER write traps would result in KVM eventually
> > seeing MSR_EFER_LMA get set and recording it in such a way that it would
> > be subsequently visible when accessing it via KVM_GET_SREGS/etc.
> >
> > However, guests kernels currently rely on MSR_EFER_LMA getting set
> > automatically when MSR_EFER_LME is set and paging is enabled via
> > CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
> > MSR_EFER_LMA even though it is set internally, and when QEMU
> > subsequently tries to pass this EFER value back to KVM via
> > KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
> > which is now considered fatal due to the aforementioned QEMU commit.
> >
> > This can be addressed by inferring the MSR_EFER_LMA bit being set when
> > paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
> > the expected bits are all present in subsequent handling on the host
> > side.
> >
> > Ultimately, this handling will be implemented in the host kernel, but to
> > avoid breaking QEMU's SEV-ES support when using older host kernels, the
> > same handling can be done in QEMU just after fetching the register
> > values via KVM_GET_SREGS*. Implement that here.
> >
> > Cc: Paolo Bonzini
> > Cc: Marcelo Tosatti
> > Cc: Tom Lendacky
> > Cc: Akihiko Odaki
> > Cc: k...@vger.kernel.org
> > Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> > Signed-off-by: Michael Roth
> > ---
> > v2:
> > - Add handling for KVM_GET_SREGS, not just KVM_GET_SREGS2
> >
> > target/i386/kvm/kvm.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 11b8177eff..8721c1bf8f 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -3610,6 +3610,7 @@ static int kvm_get_sregs(X86CPU *cpu)
> > {
> > CPUX86State *env = >env;
> > struct kvm_sregs sregs;
> > +target_ulong cr0_old;
> > int ret;
> >
> > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS, );
> > @@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
> > env->gdt.limit = sregs.gdt.limit;
> > env->gdt.base = sregs.gdt.base;
> >
> > +cr0_old = env->cr[0];
> > env->cr[0] = sregs.cr0;
> > env->cr[2] = sregs.cr2;
> > env->cr[3] = sregs.cr3;
> > env->cr[4] = sregs.cr4;
> >
> > env->efer = sregs.efer;
> > +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> > +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> > +env->efer |= MSR_EFER_LMA;
> > +}
> > +}
>
> I think that we should not check that CR0_PG has changed, and just blindly
> assume
> that if EFER.LME is set and CR0.PG is set, then EFER.LMA must be set as
> defined in x86 spec.
>
> Otherwise, suppose qemu calls kvm_get_sregs twice: First time it will work,
> but second time CR0.PG will match one that is stored in the env, and thus the
> workaround
> will not be executed, and instead we will revert back to wrong EFER value
> reported by the kernel.
>
> How about something like that:
>
>
> if (sev_es_enabled() && env->efer & MSR_EFER_LME && env->cr[0] & CR0_PG_MASK)
> {
> /*
> * Workaround KVM bug, because of which KVM might not be aware of the
> * fact that EFER.LMA was toggled by the hardware
> */
> env->efer |= MSR_EFER_LMA;
> }
Hi Maxim,
I'd already sent a v3 based on a similar suggestion from Paolo:
https://lists.gnu.org/archive/html/qemu-devel/2023-12/msg00751.html
Does that one look okay to you?
Thanks,
Mike
>
>
> Best regards,
> Maxim Levitsky
>
> >
> > /* changes to apic base and cr8/tpr are read back via
> > kvm_arch_post_run */
> > x86_update_hflags(env);
> > @@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
> > {
> > CPUX86State *env = >env;
> > struct kvm_sregs2 sregs;
> > +target_ulong cr0_old;
> > int i, ret;
> >
> > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
> > @@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
> > env->gdt.limit = sregs.gdt.limit;
> > env->gdt.base = sregs.gdt.base;
> >
> > +cr0_old = env->cr[0];
> > env->cr[0] = sregs.cr0;
> > env->cr[2] = sregs.cr2;
> > env->cr[3] = sregs.cr3;
> > env->cr[4] = sregs.cr4;
> >
> > env->efer = sregs.efer;
> > +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> > +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> > +
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Tue, 2023-12-05 at 16:28 -0600, Michael Roth wrote:
> Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
> exposed a long-running bug in current KVM support for SEV-ES where the
> kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
> kernel, in which case EFER write traps would result in KVM eventually
> seeing MSR_EFER_LMA get set and recording it in such a way that it would
> be subsequently visible when accessing it via KVM_GET_SREGS/etc.
>
> However, guests kernels currently rely on MSR_EFER_LMA getting set
> automatically when MSR_EFER_LME is set and paging is enabled via
> CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
> MSR_EFER_LMA even though it is set internally, and when QEMU
> subsequently tries to pass this EFER value back to KVM via
> KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
> which is now considered fatal due to the aforementioned QEMU commit.
>
> This can be addressed by inferring the MSR_EFER_LMA bit being set when
> paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
> the expected bits are all present in subsequent handling on the host
> side.
>
> Ultimately, this handling will be implemented in the host kernel, but to
> avoid breaking QEMU's SEV-ES support when using older host kernels, the
> same handling can be done in QEMU just after fetching the register
> values via KVM_GET_SREGS*. Implement that here.
>
> Cc: Paolo Bonzini
> Cc: Marcelo Tosatti
> Cc: Tom Lendacky
> Cc: Akihiko Odaki
> Cc: k...@vger.kernel.org
> Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> Signed-off-by: Michael Roth
> ---
> v2:
> - Add handling for KVM_GET_SREGS, not just KVM_GET_SREGS2
>
> target/i386/kvm/kvm.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 11b8177eff..8721c1bf8f 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -3610,6 +3610,7 @@ static int kvm_get_sregs(X86CPU *cpu)
> {
> CPUX86State *env = >env;
> struct kvm_sregs sregs;
> +target_ulong cr0_old;
> int ret;
>
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS, );
> @@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
> env->gdt.limit = sregs.gdt.limit;
> env->gdt.base = sregs.gdt.base;
>
> +cr0_old = env->cr[0];
> env->cr[0] = sregs.cr0;
> env->cr[2] = sregs.cr2;
> env->cr[3] = sregs.cr3;
> env->cr[4] = sregs.cr4;
>
> env->efer = sregs.efer;
> +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> +env->efer |= MSR_EFER_LMA;
> +}
> +}
I think that we should not check that CR0_PG has changed, and just blindly
assume
that if EFER.LME is set and CR0.PG is set, then EFER.LMA must be set as defined
in x86 spec.
Otherwise, suppose qemu calls kvm_get_sregs twice: First time it will work,
but second time CR0.PG will match one that is stored in the env, and thus the
workaround
will not be executed, and instead we will revert back to wrong EFER value
reported by the kernel.
How about something like that:
if (sev_es_enabled() && env->efer & MSR_EFER_LME && env->cr[0] & CR0_PG_MASK) {
/*
* Workaround KVM bug, because of which KVM might not be aware of the
* fact that EFER.LMA was toggled by the hardware
*/
env->efer |= MSR_EFER_LMA;
}
Best regards,
Maxim Levitsky
>
> /* changes to apic base and cr8/tpr are read back via kvm_arch_post_run
> */
> x86_update_hflags(env);
> @@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
> {
> CPUX86State *env = >env;
> struct kvm_sregs2 sregs;
> +target_ulong cr0_old;
> int i, ret;
>
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
> @@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
> env->gdt.limit = sregs.gdt.limit;
> env->gdt.base = sregs.gdt.base;
>
> +cr0_old = env->cr[0];
> env->cr[0] = sregs.cr0;
> env->cr[2] = sregs.cr2;
> env->cr[3] = sregs.cr3;
> env->cr[4] = sregs.cr4;
>
> env->efer = sregs.efer;
> +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> +env->efer |= MSR_EFER_LMA;
> +}
> +}
>
> env->pdptrs_valid = sregs.flags & KVM_SREGS2_FLAGS_PDPTRS_VALID;
>
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, Dec 06, 2023 at 04:04:43PM +0100, Paolo Bonzini wrote:
> On Wed, Dec 6, 2023 at 3:46 PM Michael Roth wrote:
> > > There is no need to check cr0_old or sev_es_enabled(); EFER.LMA is
> > > simply EFER.LME && CR0.PG.
> >
> > Yah, I originally had it like that, but svm_set_cr0() in the kernel only
> > sets it in vcpu->arch.efer it when setting CR0.PG, so I thought it might
> > be safer to be more selective and mirror that handling on the QEMU side
> > so we can leave as much of any other sanity checks on kernel/QEMU side
> > intact as possible. E.g., if some other bug in the kernel ends up
> > unsetting EFER.LMA while paging is still enabled, we'd still notice that
> > when passing it back in via KVM_SET_SREGS*.
> >
> > But agree it's simpler to just always set it based on CR0.PG and EFER.LMA
> > and can send a v3 if that's preferred.
>
> Yeah, in this case I think the chance of something breaking is really,
> really small.
>
> The behavior of svm_set_cr0() is more due to how the surrounding code
> looks like, than anything else.
Ok, seems reasonable. I'll plan to send a v3 with the old_cr0 stuff
dropped.
>
> > > Alternatively, sev_es_enabled() could be an assertion, that is:
> > >
> > > if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK) &&
> > >!(env->efer & MSR_EFER_LMA)) {
> > > /* Workaround for... */
> > > assert(sev_es_enabled());
> > > env->efer |= MSR_EFER_LMA;
> > > }
> > >
> > > What do you think?
> >
> > I'm a little apprehensive about this approach for similar reasons as
> > above
>
> I agree on this. I think it's worth in general to have clear
> expectations, though. If you think it's worrisome, we can commit it
> without assertion now and add it in 9.0.
I think that seems like a good approach. That would give us more time to
discuss the fix/handling on the kernel side, and then as a follow-up we
can tighten down the QEMU handling/expectations based on that.
Thanks,
Mike
>
> Paolo
>
>
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, Dec 6, 2023 at 3:46 PM Michael Roth wrote:
> > There is no need to check cr0_old or sev_es_enabled(); EFER.LMA is
> > simply EFER.LME && CR0.PG.
>
> Yah, I originally had it like that, but svm_set_cr0() in the kernel only
> sets it in vcpu->arch.efer it when setting CR0.PG, so I thought it might
> be safer to be more selective and mirror that handling on the QEMU side
> so we can leave as much of any other sanity checks on kernel/QEMU side
> intact as possible. E.g., if some other bug in the kernel ends up
> unsetting EFER.LMA while paging is still enabled, we'd still notice that
> when passing it back in via KVM_SET_SREGS*.
>
> But agree it's simpler to just always set it based on CR0.PG and EFER.LMA
> and can send a v3 if that's preferred.
Yeah, in this case I think the chance of something breaking is really,
really small.
The behavior of svm_set_cr0() is more due to how the surrounding code
looks like, than anything else.
> > Alternatively, sev_es_enabled() could be an assertion, that is:
> >
> > if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK) &&
> >!(env->efer & MSR_EFER_LMA)) {
> > /* Workaround for... */
> > assert(sev_es_enabled());
> > env->efer |= MSR_EFER_LMA;
> > }
> >
> > What do you think?
>
> I'm a little apprehensive about this approach for similar reasons as
> above
I agree on this. I think it's worth in general to have clear
expectations, though. If you think it's worrisome, we can commit it
without assertion now and add it in 9.0.
Paolo
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, Dec 06, 2023 at 02:41:13PM +0100, Paolo Bonzini wrote:
> On Tue, Dec 5, 2023 at 11:28 PM Michael Roth wrote:
> > @@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
> > env->gdt.limit = sregs.gdt.limit;
> > env->gdt.base = sregs.gdt.base;
> >
> > +cr0_old = env->cr[0];
> > env->cr[0] = sregs.cr0;
> > env->cr[2] = sregs.cr2;
> > env->cr[3] = sregs.cr3;
> > env->cr[4] = sregs.cr4;
> >
> > env->efer = sregs.efer;
> > +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> > +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> > +env->efer |= MSR_EFER_LMA;
> > +}
> > +}
>
> There is no need to check cr0_old or sev_es_enabled(); EFER.LMA is
> simply EFER.LME && CR0.PG.
Yah, I originally had it like that, but svm_set_cr0() in the kernel only
sets it in vcpu->arch.efer it when setting CR0.PG, so I thought it might
be safer to be more selective and mirror that handling on the QEMU side
so we can leave as much of any other sanity checks on kernel/QEMU side
intact as possible. E.g., if some other bug in the kernel ends up
unsetting EFER.LMA while paging is still enabled, we'd still notice that
when passing it back in via KVM_SET_SREGS*.
But agree it's simpler to just always set it based on CR0.PG and EFER.LMA
and can send a v3 if that's preferred.
>
> Alternatively, sev_es_enabled() could be an assertion, that is:
>
> if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK) &&
>!(env->efer & MSR_EFER_LMA)) {
> /* Workaround for... */
> assert(sev_es_enabled());
> env->efer |= MSR_EFER_LMA;
> }
>
> What do you think?
I'm a little apprehensive about this approach for similar reasons as
above. The current patch is guaranteed to only affect SEV-ES, whereas
this approach could trigger assertions for other edge-cases we aren't
aware of that could further impact the release. For instance, "in
theory", QEMU or KVM might have some handling where EFER.LMA is set
somewhere after (or outside of) KVM_GET_SREGS, but now with this
proposed change QEMU would become more restrictive and generate an
assert for those cases.
I don't think that's actually the case, but in the off-chance that such
a case exists there could be more fall-out such as further delays, or
the need for a stable fix. But no strong opinion there either if that
ends up being the preferred approach, just trying to consider the
pros/cons.
Thanks,
Mike
>
> Thanks,
>
> Paolo
>
> > /* changes to apic base and cr8/tpr are read back via
> > kvm_arch_post_run */
> > x86_update_hflags(env);
> > @@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
> > {
> > CPUX86State *env = >env;
> > struct kvm_sregs2 sregs;
> > +target_ulong cr0_old;
> > int i, ret;
> >
> > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
> > @@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
> > env->gdt.limit = sregs.gdt.limit;
> > env->gdt.base = sregs.gdt.base;
> >
> > +cr0_old = env->cr[0];
> > env->cr[0] = sregs.cr0;
> > env->cr[2] = sregs.cr2;
> > env->cr[3] = sregs.cr3;
> > env->cr[4] = sregs.cr4;
> >
> > env->efer = sregs.efer;
> > +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> > +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> > +env->efer |= MSR_EFER_LMA;
> > +}
> > +}
> >
> > env->pdptrs_valid = sregs.flags & KVM_SREGS2_FLAGS_PDPTRS_VALID;
> >
> > --
> > 2.25.1
> >
>
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, Dec 6, 2023 at 2:13 PM Michael Roth wrote:
> > This 'Fixes:' tag is misleading, since as you mentioned this commit
> > only exposes the issue.
>
> That's true, a "Workaround-for: " tag or something like that might be more
> appropriate. I just wanted to make it clear that SEV-ES support is no longer
> working with that patch applied, so I used Fixes: and elaborated on the
> commit message. I can change it if there's a better way to convey this
> though.
That's fine, Fixes is also for automated checks, like "if you have
this commit you also want this one".
> >
> > Commit d499f196fe ("target/i386: Added consistency checks for EFER")
> > or around it seems more appropriate.
>
> Those checks seem to be more for TCG.
Yes, that's 100% TCG code.
> The actual bug is in the host
> kernel, and it seems to have been there basically since the original
> SEV-ES host support went in in 2020. I've also sent a patch to address
> this in KVM:
>
>
> https://lore.kernel.org/lkml/20231205234956.1156210-1-michael.r...@amd.com/T/#u
Thanks, looking at it.
Paolo
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Tue, Dec 5, 2023 at 11:28 PM Michael Roth wrote:
> @@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
> env->gdt.limit = sregs.gdt.limit;
> env->gdt.base = sregs.gdt.base;
>
> +cr0_old = env->cr[0];
> env->cr[0] = sregs.cr0;
> env->cr[2] = sregs.cr2;
> env->cr[3] = sregs.cr3;
> env->cr[4] = sregs.cr4;
>
> env->efer = sregs.efer;
> +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> +env->efer |= MSR_EFER_LMA;
> +}
> +}
There is no need to check cr0_old or sev_es_enabled(); EFER.LMA is
simply EFER.LME && CR0.PG.
Alternatively, sev_es_enabled() could be an assertion, that is:
if ((env->efer & MSR_EFER_LME) && (env->cr[0] & CR0_PG_MASK) &&
!(env->efer & MSR_EFER_LMA)) {
/* Workaround for... */
assert(sev_es_enabled());
env->efer |= MSR_EFER_LMA;
}
What do you think?
Thanks,
Paolo
> /* changes to apic base and cr8/tpr are read back via kvm_arch_post_run
> */
> x86_update_hflags(env);
> @@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
> {
> CPUX86State *env = >env;
> struct kvm_sregs2 sregs;
> +target_ulong cr0_old;
> int i, ret;
>
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
> @@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
> env->gdt.limit = sregs.gdt.limit;
> env->gdt.base = sregs.gdt.base;
>
> +cr0_old = env->cr[0];
> env->cr[0] = sregs.cr0;
> env->cr[2] = sregs.cr2;
> env->cr[3] = sregs.cr3;
> env->cr[4] = sregs.cr4;
>
> env->efer = sregs.efer;
> +if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
> +if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
> +env->efer |= MSR_EFER_LMA;
> +}
> +}
>
> env->pdptrs_valid = sregs.flags & KVM_SREGS2_FLAGS_PDPTRS_VALID;
>
> --
> 2.25.1
>
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
On Wed, Dec 06, 2023 at 12:48:35PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Michael,
>
> (Cc'ing Lara, Vitaly and Maxim)
>
> On 5/12/23 23:28, Michael Roth wrote:
> > Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
> > added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
> > exposed a long-running bug in current KVM support for SEV-ES where the
> > kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
> > kernel, in which case EFER write traps would result in KVM eventually
> > seeing MSR_EFER_LMA get set and recording it in such a way that it would
> > be subsequently visible when accessing it via KVM_GET_SREGS/etc.
> >
> > However, guests kernels currently rely on MSR_EFER_LMA getting set
> > automatically when MSR_EFER_LME is set and paging is enabled via
> > CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
> > MSR_EFER_LMA even though it is set internally, and when QEMU
> > subsequently tries to pass this EFER value back to KVM via
> > KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
> > which is now considered fatal due to the aforementioned QEMU commit.
> >
> > This can be addressed by inferring the MSR_EFER_LMA bit being set when
> > paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
> > the expected bits are all present in subsequent handling on the host
> > side.
> >
> > Ultimately, this handling will be implemented in the host kernel, but to
> > avoid breaking QEMU's SEV-ES support when using older host kernels, the
> > same handling can be done in QEMU just after fetching the register
> > values via KVM_GET_SREGS*. Implement that here.
> >
> > Cc: Paolo Bonzini
> > Cc: Marcelo Tosatti
> > Cc: Tom Lendacky
> > Cc: Akihiko Odaki
> > Cc: k...@vger.kernel.org
> > Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
>
Hi Philippe,
> This 'Fixes:' tag is misleading, since as you mentioned this commit
> only exposes the issue.
That's true, a "Workaround-for: " tag or something like that might be more
appropriate. I just wanted to make it clear that SEV-ES support is no longer
working with that patch applied, so I used Fixes: and elaborated on the
commit message. I can change it if there's a better way to convey this
though.
>
> Commit d499f196fe ("target/i386: Added consistency checks for EFER")
> or around it seems more appropriate.
Those checks seem to be more for TCG. The actual bug is in the host
kernel, and it seems to have been there basically since the original
SEV-ES host support went in in 2020. I've also sent a patch to address
this in KVM:
https://lore.kernel.org/lkml/20231205234956.1156210-1-michael.r...@amd.com/T/#u
but in the meantime it means that QEMU 8.2+ SEV-ES support would no
longer work for any current/older host kernels, so I'm hoping a targeted
workaround is warranted to cover that gap.
>
> Is this feature easily testable on our CI, on a x86 runner with KVM
> access?
SEV-ES support was introduced with EPYC Zen2 architecture (EPYC 7002
series processors, aka "Rome"). If there are any systems in the test pool
that are Zen2 or greater, then a simple boot of a SEV-ES linux guest would
be enough to trigger the QEMU crash. I'm not sure if there are any systems
of that sort in the pool though.
Thanks,
Mike
Re: [PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
Hi Michael,
(Cc'ing Lara, Vitaly and Maxim)
On 5/12/23 23:28, Michael Roth wrote:
Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
exposed a long-running bug in current KVM support for SEV-ES where the
kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
kernel, in which case EFER write traps would result in KVM eventually
seeing MSR_EFER_LMA get set and recording it in such a way that it would
be subsequently visible when accessing it via KVM_GET_SREGS/etc.
However, guests kernels currently rely on MSR_EFER_LMA getting set
automatically when MSR_EFER_LME is set and paging is enabled via
CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
MSR_EFER_LMA even though it is set internally, and when QEMU
subsequently tries to pass this EFER value back to KVM via
KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
which is now considered fatal due to the aforementioned QEMU commit.
This can be addressed by inferring the MSR_EFER_LMA bit being set when
paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
the expected bits are all present in subsequent handling on the host
side.
Ultimately, this handling will be implemented in the host kernel, but to
avoid breaking QEMU's SEV-ES support when using older host kernels, the
same handling can be done in QEMU just after fetching the register
values via KVM_GET_SREGS*. Implement that here.
Cc: Paolo Bonzini
Cc: Marcelo Tosatti
Cc: Tom Lendacky
Cc: Akihiko Odaki
Cc: k...@vger.kernel.org
Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
This 'Fixes:' tag is misleading, since as you mentioned this commit
only exposes the issue.
Commit d499f196fe ("target/i386: Added consistency checks for EFER")
or around it seems more appropriate.
Is this feature easily testable on our CI, on a x86 runner with KVM
access?
Signed-off-by: Michael Roth
---
v2:
- Add handling for KVM_GET_SREGS, not just KVM_GET_SREGS2
target/i386/kvm/kvm.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 11b8177eff..8721c1bf8f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3610,6 +3610,7 @@ static int kvm_get_sregs(X86CPU *cpu)
{
CPUX86State *env = >env;
struct kvm_sregs sregs;
+target_ulong cr0_old;
int ret;
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS, );
@@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
env->gdt.limit = sregs.gdt.limit;
env->gdt.base = sregs.gdt.base;
+cr0_old = env->cr[0];
env->cr[0] = sregs.cr0;
env->cr[2] = sregs.cr2;
env->cr[3] = sregs.cr3;
env->cr[4] = sregs.cr4;
env->efer = sregs.efer;
+if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
+if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
+env->efer |= MSR_EFER_LMA;
+}
+}
/* changes to apic base and cr8/tpr are read back via kvm_arch_post_run */
x86_update_hflags(env);
@@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
{
CPUX86State *env = >env;
struct kvm_sregs2 sregs;
+target_ulong cr0_old;
int i, ret;
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
@@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
env->gdt.limit = sregs.gdt.limit;
env->gdt.base = sregs.gdt.base;
+cr0_old = env->cr[0];
env->cr[0] = sregs.cr0;
env->cr[2] = sregs.cr2;
env->cr[3] = sregs.cr3;
env->cr[4] = sregs.cr4;
env->efer = sregs.efer;
+if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
+if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
+env->efer |= MSR_EFER_LMA;
+}
+}
env->pdptrs_valid = sregs.flags & KVM_SREGS2_FLAGS_PDPTRS_VALID;
[PATCH v2 for-8.2?] i386/sev: Avoid SEV-ES crash due to missing MSR_EFER_LMA bit
Commit 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
added error checking for KVM_SET_SREGS/KVM_SET_SREGS2. In doing so, it
exposed a long-running bug in current KVM support for SEV-ES where the
kernel assumes that MSR_EFER_LMA will be set explicitly by the guest
kernel, in which case EFER write traps would result in KVM eventually
seeing MSR_EFER_LMA get set and recording it in such a way that it would
be subsequently visible when accessing it via KVM_GET_SREGS/etc.
However, guests kernels currently rely on MSR_EFER_LMA getting set
automatically when MSR_EFER_LME is set and paging is enabled via
CR0_PG_MASK. As a result, the EFER write traps don't actually expose the
MSR_EFER_LMA even though it is set internally, and when QEMU
subsequently tries to pass this EFER value back to KVM via
KVM_SET_SREGS* it will fail various sanity checks and return -EINVAL,
which is now considered fatal due to the aforementioned QEMU commit.
This can be addressed by inferring the MSR_EFER_LMA bit being set when
paging is enabled and MSR_EFER_LME is set, and synthesizing it to ensure
the expected bits are all present in subsequent handling on the host
side.
Ultimately, this handling will be implemented in the host kernel, but to
avoid breaking QEMU's SEV-ES support when using older host kernels, the
same handling can be done in QEMU just after fetching the register
values via KVM_GET_SREGS*. Implement that here.
Cc: Paolo Bonzini
Cc: Marcelo Tosatti
Cc: Tom Lendacky
Cc: Akihiko Odaki
Cc: k...@vger.kernel.org
Fixes: 7191f24c7fcf ("accel/kvm/kvm-all: Handle register access errors")
Signed-off-by: Michael Roth
---
v2:
- Add handling for KVM_GET_SREGS, not just KVM_GET_SREGS2
target/i386/kvm/kvm.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 11b8177eff..8721c1bf8f 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -3610,6 +3610,7 @@ static int kvm_get_sregs(X86CPU *cpu)
{
CPUX86State *env = >env;
struct kvm_sregs sregs;
+target_ulong cr0_old;
int ret;
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS, );
@@ -3637,12 +3638,18 @@ static int kvm_get_sregs(X86CPU *cpu)
env->gdt.limit = sregs.gdt.limit;
env->gdt.base = sregs.gdt.base;
+cr0_old = env->cr[0];
env->cr[0] = sregs.cr0;
env->cr[2] = sregs.cr2;
env->cr[3] = sregs.cr3;
env->cr[4] = sregs.cr4;
env->efer = sregs.efer;
+if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
+if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
+env->efer |= MSR_EFER_LMA;
+}
+}
/* changes to apic base and cr8/tpr are read back via kvm_arch_post_run */
x86_update_hflags(env);
@@ -3654,6 +3661,7 @@ static int kvm_get_sregs2(X86CPU *cpu)
{
CPUX86State *env = >env;
struct kvm_sregs2 sregs;
+target_ulong cr0_old;
int i, ret;
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_SREGS2, );
@@ -3676,12 +3684,18 @@ static int kvm_get_sregs2(X86CPU *cpu)
env->gdt.limit = sregs.gdt.limit;
env->gdt.base = sregs.gdt.base;
+cr0_old = env->cr[0];
env->cr[0] = sregs.cr0;
env->cr[2] = sregs.cr2;
env->cr[3] = sregs.cr3;
env->cr[4] = sregs.cr4;
env->efer = sregs.efer;
+if (sev_es_enabled() && env->efer & MSR_EFER_LME) {
+if (!(cr0_old & CR0_PG_MASK) && env->cr[0] & CR0_PG_MASK) {
+env->efer |= MSR_EFER_LMA;
+}
+}
env->pdptrs_valid = sregs.flags & KVM_SREGS2_FLAGS_PDPTRS_VALID;
--
2.25.1
