linux-kernel - Re: [PATCH v5] KVM: x86: avoid large stack allocations in em

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]

Message-ID: <1815926933.3699401.1496228489339.JavaMail.zimbra@redhat.com>
Date:   Wed, 31 May 2017 07:01:29 -0400 (EDT)
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Nick Desaulniers <nick.desaulniers@...il.com>
Cc:     Radim Krčmář <rkrcmar@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] KVM: x86: avoid large stack allocations in
 em_fxrstor


> +		size = offsetof(struct fxregs_state, xmm_space[16]);

This still has the same issue (it should be multiplied by 4).  Here's my
take on it; I checked the compiled code and it's pretty good too (the
compiler knows to do the fxsave if and only if ctxt->mode <
X86EMUL_MODE_PROT64, because that's when the size is smaller):

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 55470ad07c2a..b76f19d2684d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3944,10 +3944,19 @@ static int check_fxsr(struct x86_emulate_ctxt *ctxt)
  * Hardware doesn't save and restore XMM 0-7 without CR4.OSFXSR, but does save
  * and restore MXCSR.
  */
-static size_t xmm_offset(struct x86_emulate_ctxt *ctxt)
+static size_t __fxstate_size(int nregs)
 {
-	bool b = ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSFXSR;
-	return offsetof(struct fxregs_state, xmm_space[b ? 8 * 16 / 4 : 0]);
+	return offsetof(struct fxregs_state, xmm_space[0]) + nregs * 16;
+}
+
+static inline size_t fxstate_size(struct x86_emulate_ctxt *ctxt)
+{
+	bool cr4_osfxsr;
+	if (ctxt->mode == X86EMUL_MODE_PROT64)
+		return __fxstate_size(16);
+
+	cr4_osfxsr = ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSFXSR;
+	return __fxstate_size(cr4_osfxsr ? 8 : 0);
 }
 
 /*
@@ -3987,7 +3996,7 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt)
 		return rc;
 
 	return segmented_write_std(ctxt, ctxt->memop.addr.mem, &fx_state,
-		xmm_offset(ctxt));
+		                   fxstate_size(ctxt));
 }
 
 static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
@@ -4002,13 +4011,11 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
 
 	ctxt->ops->get_fpu(ctxt);
 
-	if (ctxt->mode < X86EMUL_MODE_PROT64) {
+	size = fxstate_size(ctxt);
+	if (size < __fxstate_size(16)) {
 		rc = asm_safe("fxsave %[fx]", , [fx] "+m"(fx_state));
 		if (rc != X86EMUL_CONTINUE)
 			goto out;
-		size = xmm_offset(ctxt);
-	} else {
-		size = offsetof(struct fxregs_state, xmm_space[16]);
 	}
 
 	rc = segmented_read_std(ctxt, ctxt->memop.addr.mem, &fx_state, size);

Thanks Nick for the patches and Radim for the reviews!

Paolo