Re: [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM

On Wed, 2021-06-23 at 15:32 +0200, Vitaly Kuznetsov wrote:

Maxim Levitsky [off-list ref] writes:

quoted

On Wed, 2021-06-23 at 16:01 +0300, Maxim Levitsky wrote:

quoted

On Wed, 2021-06-23 at 11:39 +0200, Paolo Bonzini wrote:

quoted

On 23/06/21 09:44, Vitaly Kuznetsov wrote:

quoted

- RFC: I'm not 100% sure my 'smart' idea to use currently-
unused HSAVE area
is that smart. Also, we don't even seem to check that L1 set
it up upon
nested VMRUN so hypervisors which don't do that may remain
broken. A very
much needed selftest is also missing.

It's certainly a bit weird, but I guess it counts as smart
too.  It 
needs a few more comments, but I think it's a good solution.

One could delay the backwards memcpy until vmexit time, but
that would 
require a new flag so it's not worth it for what is a pretty
rare and 
already expensive case.

Paolo

Hi!

I did some homework on this now and I would like to share few my
thoughts on this:

First of all my attention caught the way we intercept the #SMI
(this isn't 100% related to the bug but still worth talking about
IMHO)

A. Bare metal: Looks like SVM allows to intercept SMI, with
SVM_EXIT_SMI, 
 with an intention of then entering the BIOS SMM handler manually
using the SMM_CTL msr.
 On bare metal we do set the INTERCEPT_SMI but we emulate the
exit as a nop.
 I guess on bare metal there are some undocumented bits that BIOS
set which
 make the CPU to ignore that SMI intercept and still take the
#SMI handler,
 normally but I wonder if we could still break some motherboard
 code due to that.


B. Nested: If #SMI is intercepted, then it causes nested VMEXIT.
 Since KVM does enable SMI intercept, when it runs nested it
means that all SMIs 
 that nested KVM gets are emulated as NOP, and L1's SMI handler
is not run.


About the issue that was fixed in this patch. Let me try to
understand how
it would work on bare metal:

1. A guest is entered. Host state is saved to VM_HSAVE_PA area
(or stashed somewhere
  in the CPU)

2. #SMI (without intercept) happens

3. CPU has to exit SVM, and start running the host SMI handler,
it loads the SMM
    state without touching the VM_HSAVE_PA runs the SMI handler,
then once it RSMs,
    it restores the guest state from SMM area and continues the
guest

4. Once a normal VMexit happens, the host state is restored from
VM_HSAVE_PA

So host state indeed can't be saved to VMC01.

I to be honest think would prefer not to use the L1's hsave area
but rather add back our
'hsave' in KVM and store there the L1 host state on the nested
entry always.

This way we will avoid touching the vmcb01 at all and both solve
the issue and 
reduce code complexity.
(copying of L1 host state to what basically is L1 guest state
area and back
even has a comment to explain why it (was) possible to do so.
(before you discovered that this doesn't work with SMM).

I need more coffee today. The comment is somwhat wrong actually.
When L1 switches to L2, then its HSAVE area is L1 guest state, but
but L1 is a "host" vs L2, so it is host state.
The copying is more between kvm's register cache and the vmcb.

So maybe backing it up as this patch does is the best solution yet.
I will take more in depth look at this soon.

We can resurrect 'hsave' and keep it internally indeed but to make
this
migratable, we'd have to add it to the nested state acquired through
svm_get_nested_state(). Using L1's HSAVE area (ponted to by
MSR_VM_HSAVE_PA) avoids that as we have everything in L1's memory.
And,

Hi!

I think I would prefer to avoid touching guest memory as much
as possible to avoid the shenanigans of accessing it:

For example on nested state read we are not allowed to write guest
memory since at the point it is already migrated, and for setting
nested state we are not allowed to even read the guest memory since
the memory map might not be up to date. Then a malicious guest can
always change its memory which also can cause issues.

Since it didn't work before and both sides of migration need a fix,
adding a new flag and adding hsave area to nested state seems like a
very good thing.

I think though that I would use that smm hsave area just like you
did in the patch, just not save it to the guest memory and migrate
it as a new state.

I would call it something smm_l1_hsave_area or something like
that with a comment explaining why it is needed.

This way we still avoid overhead of copying the hsave area
on each nested entry.

What do you think?

Best regards,
	Maxim Levitsky

as far as I understand, we comply with the spec as 1) L1 has to set
it
up and 2) L1 is not supposed to expect any particular format there,
it's
completely volatile.