linux-kernel - Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of page walk

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:   Tue, 13 Dec 2016 18:09:57 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Brijesh Singh <brijesh.singh@....com>
Cc:     kvm@...r.kernel.org, thomas lendacky <thomas.lendacky@....com>,
        rkrcmar@...hat.com, joro@...tes.org, x86@...nel.org,
        linux-kernel@...r.kernel.org, mingo@...hat.com, hpa@...or.com,
        tglx@...utronix.de, bp@...e.de
Subject: Re: [PATCH v2 3/3] kvm: svm: Use the hardware provided GPA instead of
 page walk



On 12/12/2016 18:51, Brijesh Singh wrote:
> As per the AMD BKDG [1] Section 2.7.1, we should not be using any of
> these instruction for MMIO access, the behavior is undefined.
> 
> The question is, do we really need to add logic to detect the cross-page
> MMIO accesses and push/pop mem operations so that we pass the
> kvm-unit-test or we should update the unit test? Like you said
> cross-page MMIO access detection is going to be a bit tricky.

Actually there is a nice trick you can do to support cross-page
MMIO access detection:

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 37cd31645d45..754d251dc611 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4549,6 +4549,7 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
 	 */
 	if (vcpu->arch.gpa_available &&
 	    !emulator_is_string_op(ctxt) &&
+	    (addr & ~PAGE_MASK) == (exception->address & ~PAGE_MASK) &&
 	    vcpu_is_mmio_gpa(vcpu, addr, exception->address, write)) {
 		gpa = exception->address;
 		goto mmio;


It fixes the testcase for push/pop with two memory ops too,
but it's not reliable, so your change for TwoMemOp is still
necessary.  Feel free to include it in your patch!

Regarding the replacement of emulator_is_string_op with
emulator_is_two_memory_op, what about REP prefixes?  In that
case I think that you do need to reject string ops.  So the
function would have to reject all TwoMemOps, and REP-prefixed
String operations.

Paolo