linux-kernel - Re: [PATCH 3/5] KVM: nSVM: Don't forget about L1-injected events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Apr 2022 00:08:26 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Brijesh Singh <brijesh.singh@....com>,
        Jon Grimm <Jon.Grimm@....com>,
        David Kaplan <David.Kaplan@....com>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Liam Merwick <liam.merwick@...cle.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] KVM: nSVM: Don't forget about L1-injected events

On Fri, Apr 01, 2022, Maciej S. Szmigiero wrote:
> On 31.03.2022 01:20, Sean Christopherson wrote:
> > Re-executing the INTn is wrong, the instruction has already completed decode and
> > execution.  E.g. if there's there's a code breakpoint on the INTn, rewinding will
> > cause a spurious #DB.
> > 
> > KVM's INT3 shenanigans are bonkers, but I guess there's no better option given
> > that the APM says "Software interrupts cannot be properly injected if the processor
> > does not support the NextRIP field.".  What a mess.
> 
> Note that KVM currently always tries to re-execute the current instruction
> when asked to re-inject a #BP or a #OF, even when nrips are enabled.

Yep, and my vote is to fix that.

> Also, #BP (and #OF, too) is returned as type SVM_EXITINTINFO_TYPE_EXEPT,
> not as SVM_EXITINTINFO_TYPE_SOFT (soft interrupt), so it should be
> re-injected accordingly.

Ahhh, SVM doesn't differentiate between software exceptions and hardware exceptions.
Finally found the relevant section in the APM:

  Despite the instruction name, the events raised by the INT1 (also known as ICEBP),
  INT3 and INTO instructions (opcodes F1h, CCh and CEh) are considered exceptions for
  the purposes of EXITINTINFO, not software interrupts. Only events raised by the INTn
  instruction (opcode CDh) are considered software interrupts.

VMX has separate identifiers for software interrupts and for software exceptions,
where as SVM unconditionally treats #BP and #OF as soft:

  Injecting an exception (TYPE = 3) with vectors 3 or 4 behaves like a trap raised by
  INT3 and INTO instructions

Now I'm curious why Intel doesn't do the same...

> > Anyways, for the common nrips=true case, I strongly prefer that we properly fix
> > the non-nested case and re-inject software interrupts, which should in turn
> > naturally fix this nested case.
> 
> This would also need making the #BP or #OF current instruction
> re-execution conditional on (at least) nrips support.
> 
> I am not sure, however, whether this won't introduce any regressions.
> That's why this patch set changed the behavior here only for the
> L1 -> L2 case.
> 
> Another issue is whether a L1 hypervisor can legally inject a #VC
> into its L2 (since these are never re-injected).

I would expect to work, and it's easy to find out.  I know VMX allows injecting
non-existent exceptions, but the APM is vague as usual and says VMRUN will fail...

  If the VMM attempts to inject an event that is impossible for the guest mode

> We still need L1 -> L2 event injection detection to restore the NextRIP
> field when re-injecting an event that uses it.

You lost me on this one.  KVM L0 is only (and always!) responsible for saving the
relevant info into vmcb12, why does it need to detect where the vectoring exception
came from?

> > And for nrips=false, my vote is to either punt
> > and document it as a "KVM erratum", or straight up make nested require nrips.
> 
> A quick Internet search shows that the first CPUs with NextRIP were the
> second-generation Family 10h CPUs (Phenom II, Athlon II, etc.).
> They started being released in early 2009, so we probably don't need to
> worry about the non-nrips case too much.
> 
> For the nested case, orthodox reading of the aforementioned APM sentence
> would mean that a L1 hypervisor is not allowed either to make use of such
> event injection in the non-nrips case.

Heh, my reading of it is that it's not disallowed, it just won't work correctly,
i.e. the INTn won't be skipped.