virtualization.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Joerg Roedel <joro@8bytes.org>
To: Andy Lutomirski <luto@kernel.org>
Cc: kvm list <kvm@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Linux Virtualization <virtualization@lists.linux-foundation.org>,
	Arvind Sankar <nivedita@alum.mit.edu>,
	"H. Peter Anvin" <hpa@zytor.com>, Jiri Slaby <jslaby@suse.cz>,
	X86 ML <x86@kernel.org>, David Rientjes <rientjes@google.com>,
	Martin Radev <martin.b.radev@gmail.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Joerg Roedel <jroedel@suse.de>, Kees Cook <keescook@chromium.org>,
	Cfir Cohen <cfir@google.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Juergen Gross <jgross@suse.com>, Mike Stunes <mstunes@vmware.com>,
	Sean Christopherson <seanjc@google.com>,
	LKML <linux-kernel@vger.kernel.org>,
	stable <stable@vger.kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Erdem Aktas <erdemaktas@google.com>
Subject: Re: [PATCH 2/3] x86/sev-es: Check if regs->sp is trusted before adjusting #VC IST stack
Date: Thu, 18 Feb 2021 12:25:00 +0100	[thread overview]
Message-ID: <20210218112500.GH7302@8bytes.org> (raw)
In-Reply-To: <CALCETrWw-we3O4_upDoXJ4NzZHsBqNO69ht6nBp3y+QFhwPgKw@mail.gmail.com>

Hi Andy,

On Wed, Feb 17, 2021 at 10:09:46AM -0800, Andy Lutomirski wrote:
> Can you get rid of the linked list hack while you're at it?  This code
> is unnecessarily convoluted right now, and it seems to be just asking
> for weird bugs.  Just stash the old value in a local variable, please.

Yeah, the linked list is not really necessary right now, because of the
way nested NMI handling works and given that these functions are only
used in the NMI handler right now.
The whole #VC handling code was written with future requirements in
mind, like what is needed when debugging registers get virtualized and
#HV gets enabled.
Until its clear whether __sev_es_ist_enter/exit() is needed in any of
these paths, I'd like to keep the linked list for now. It is more
complicated but allows nesting.

> Meanwhile, I'm pretty sure I can break this whole scheme if the
> hypervisor is messing with us.  As a trivial example, the sequence
> SYSCALL gap -> #VC -> NMI -> #VC will go quite poorly.

I don't see how this would break, can you elaborate?

What I think happens is:

SYSCALL gap (RSP is from userspace and untrusted)

	-> #VC - Handler on #VC IST stack detects that it interrupted
	   the SYSCALL gap and switches to the task stack.


	-> NMI - Now running on NMI IST stack. Depending on whether the
	   stack switch in the #VC handler already happened, the #VC IST
	   entry is adjusted so that a subsequent #VC will not overwrite
	   the interrupted handlers stack frame.

	-> #VC - Handler runs on the adjusted #VC IST stack and switches
	   itself back to the NMI IST stack. This is safe wrt. nested
	   NMIs as long as nested NMIs itself are safe.

As a rule of thumb, think of the #VC handler as trying to be a non-IST
handler by switching itself to the interrupted stack or the task stack.
If it detects that this is not possible (which can't happen right now,
but with SNP), it will kill the guest.

Also #VC is currently not safe against #MC, but this is the same as with
NMI and #MC. And more care is needed when SNP gets enabled and #VCs can
happen in the stack switching/stack adjustment code itself. I will
probably just add a check then to kill the guest if an SNP related #VC
comes from noinstr code.

> Is this really better than just turning IST off for #VC and
> documenting that we are not secure against a malicious hypervisor yet?

It needs to be IST, even without SNP, because #DB is IST too. When the
hypervisor intercepts #DB then any #DB exception will be turned into
#VC, so #VC needs to be handled anywhere a #DB can happen.

And with SNP we need to be able to at least detect a malicious HV so we
can reliably kill the guest. Otherwise the HV could potentially take
control over the guest's execution flow and make it reveal its secrets.

Regards,

	Joerg
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

  reply	other threads:[~2021-02-18 11:25 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-17 12:01 [PATCH 0/3] x86/sev-es: Check for trusted regs->sp in __sev_es_ist_enter() Joerg Roedel
2021-02-17 12:01 ` [PATCH 1/3] x86/sev-es: Introduce from_syscall_gap() helper Joerg Roedel
2021-02-17 17:59   ` Borislav Petkov
2021-02-17 12:01 ` [PATCH 2/3] x86/sev-es: Check if regs->sp is trusted before adjusting #VC IST stack Joerg Roedel
2021-02-17 18:00   ` Borislav Petkov
2021-02-17 18:09   ` Andy Lutomirski
2021-02-18 11:25     ` Joerg Roedel [this message]
2021-02-18 17:49       ` Andy Lutomirski
2021-02-18 19:21         ` Joerg Roedel
2021-02-19  0:28           ` Andy Lutomirski
2021-02-19 11:05             ` Joerg Roedel
2021-02-17 12:01 ` [PATCH 3/3] x86/sev-es: Improve comments in and around __sev_es_ist_enter/exit() Joerg Roedel
2021-02-17 18:00   ` Borislav Petkov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210218112500.GH7302@8bytes.org \
    --to=joro@8bytes.org \
    --cc=cfir@google.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=erdemaktas@google.com \
    --cc=hpa@zytor.com \
    --cc=jgross@suse.com \
    --cc=jroedel@suse.de \
    --cc=jslaby@suse.cz \
    --cc=keescook@chromium.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=martin.b.radev@gmail.com \
    --cc=mhiramat@kernel.org \
    --cc=mstunes@vmware.com \
    --cc=nivedita@alum.mit.edu \
    --cc=peterz@infradead.org \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=stable@vger.kernel.org \
    --cc=thomas.lendacky@amd.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).