On Wed, Feb 17, 2021 at 4:02 AM Joerg Roedel joro@8bytes.org wrote:
From: Joerg Roedel jroedel@suse.de
The code in the NMI handler to adjust the #VC handler IST stack is needed in case an NMI hits when the #VC handler is still using its IST stack. But the check for this condition also needs to look if the regs->sp value is trusted, meaning it was not set by user-space. Extend the check to not use regs->sp when the NMI interrupted user-space code or the SYSCALL gap.
Reported-by: Andy Lutomirski luto@kernel.org Fixes: 315562c9af3d5 ("x86/sev-es: Adjust #VC IST Stack on entering NMI handler") Cc: stable@vger.kernel.org # 5.10+ Signed-off-by: Joerg Roedel jroedel@suse.de
arch/x86/kernel/sev-es.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/sev-es.c b/arch/x86/kernel/sev-es.c index 84c1821819af..0df38b185d53 100644 --- a/arch/x86/kernel/sev-es.c +++ b/arch/x86/kernel/sev-es.c @@ -144,7 +144,9 @@ void noinstr __sev_es_ist_enter(struct pt_regs *regs) old_ist = __this_cpu_read(cpu_tss_rw.x86_tss.ist[IST_INDEX_VC]);
/* Make room on the IST stack */
if (on_vc_stack(regs->sp))
if (on_vc_stack(regs->sp) &&!user_mode(regs) &&!from_syscall_gap(regs)) new_ist = ALIGN_DOWN(regs->sp, 8) - sizeof(old_ist); else
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.
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. Is this really better than just turning IST off for #VC and documenting that we are not secure against a malicious hypervisor yet?
--Andy