linux-kernel - Re: [V3 10/11] KVM: selftests: Add ucall pool based implementation

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Yv/QPxeKczzmxR9H@google.com>
Date:   Fri, 19 Aug 2022 18:02:39 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Andrew Jones <andrew.jones@...ux.dev>
Cc:     Peter Gonda <pgonda@...gle.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, marcorr@...gle.com,
        michael.roth@....com, thomas.lendacky@....com, joro@...tes.org,
        mizhang@...gle.com, pbonzini@...hat.com, vannapurve@...gle.com
Subject: Re: [V3 10/11] KVM: selftests: Add ucall pool based implementation

On Fri, Aug 19, 2022, Andrew Jones wrote:
> On Thu, Aug 18, 2022 at 11:29:11PM +0000, Sean Christopherson wrote:
> > On Thu, Aug 18, 2022, Andrew Jones wrote:
> Dropping the only once-used uc_pool_idx() and adding the comment looks
> better, but I don't think there was a bug because uc == uc->hva.

uc == uc->hva for the host, but not for the guest.  From the guest's perspective,
"hva" is an opaque pointer that has no direct relation to "uc".

> 1) Doing ucall_init() at VM create time may be too early for Arm. Arm
> probes for an unmapped address for the MMIO address it needs, so it's
> best to do that after all mapping.

Argh.  I really hate the MMIO probing.  Is there really no arbitrary address that
selftests can carve out and simply say "thou shalt not create a memslot here"?

Or can we just say that it's always immediate after memslot0?  That would allow
us to delete the searching code in ARM's ucall_arch_init().

> 2) We'll need to change the sanity checks in Arm's get_ucall() to not
> check that the MMIO address == ucall_exit_mmio_addr since there's no
> guarantee that the exiting guest's address matches whatever is lingering
> in the host's version. We can either drop the sanity check altogether
> or try to come up with something else.

Ah, right.  This one at least is easy to handle.  If the address is per-VM, just
track the address.  If it's hardcoded (as a fix for #1) then, it's even simpler.

diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h
index 8623c1568f97..74167540815b 100644
--- a/tools/testing/selftests/kvm/include/kvm_util_base.h
+++ b/tools/testing/selftests/kvm/include/kvm_util_base.h
@@ -94,7 +94,8 @@ struct kvm_vm {
        vm_paddr_t pgd;
        vm_vaddr_t gdt;
        vm_vaddr_t tss;
-       vm_vaddr_t idt;
+       vm_paddr_t pgd;
+       vm_vaddr_t ucall_addr;
        vm_vaddr_t handlers;
        uint32_t dirty_ring_size;
        struct vm_memcrypt memcrypt;
diff --git a/tools/testing/selftests/kvm/lib/aarch64/ucall.c b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
index b5d904f074b6..7f1d50dab0df 100644
--- a/tools/testing/selftests/kvm/lib/aarch64/ucall.c
+++ b/tools/testing/selftests/kvm/lib/aarch64/ucall.c
@@ -14,6 +14,8 @@ static vm_vaddr_t *ucall_exit_mmio_addr;

 static void ucall_set_mmio_addr(struct kvm_vm *vm, vm_vaddr_t val)
 {
+       vm->ucall_addr = val;
+
        atomic_sync_global_pointer_to_guest(vm, ucall_exit_mmio_addr, val);
 }

@@ -87,7 +89,7 @@ void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
        struct kvm_run *run = vcpu->run;

        if (run->exit_reason == KVM_EXIT_MMIO &&
-           run->mmio.phys_addr == (uint64_t)ucall_exit_mmio_addr) {
+           run->mmio.phys_addr == vcpu->vm->ucall_addr) {
                TEST_ASSERT(run->mmio.is_write && run->mmio.len == sizeof(uint64_t),
                            "Unexpected ucall exit mmio address access");
                return (void *)(*((uint64_t *)run->mmio.data));