* [PATCH] KVM: arm64: Selftest for pKVM transitions
@ 2024-11-29 12:58 Quentin Perret
2024-12-20 14:13 ` Will Deacon
0 siblings, 1 reply; 3+ messages in thread
From: Quentin Perret @ 2024-11-29 12:58 UTC (permalink / raw)
To: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, Will Deacon
Cc: linux-arm-kernel, kvmarm, linux-kernel, qperret
We have recently found a bug [1] in the pKVM memory ownership
transitions by code inspection, but it could have been caught with a
test.
Introduce a boot-time selftest exercising all the known pKVM memory
transitions and importantly checks the rejection of illegal transitions.
The new test is hidden behind a new Kconfig option separate from
CONFIG_EL2_NVHE_DEBUG on purpose as that has side effects on the
transition checks ([1] doesn't reproduce with EL2 debug enabled).
[1] https://lore.kernel.org/kvmarm/20241128154406.602875-1-qperret@google.com/
Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Quentin Perret <qperret@google.com>
---
arch/arm64/kvm/Kconfig | 10 ++
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 6 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 110 ++++++++++++++++++
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +
4 files changed, 128 insertions(+)
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index ead632ad01b4..038d7f52232c 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -46,6 +46,7 @@ menuconfig KVM
config NVHE_EL2_DEBUG
bool "Debug mode for non-VHE EL2 object"
depends on KVM
+ select PKVM_SELFTESTS
help
Say Y here to enable the debug mode for the non-VHE KVM EL2 object.
Failure reports will BUG() in the hypervisor. This is intended for
@@ -53,6 +54,15 @@ config NVHE_EL2_DEBUG
If unsure, say N.
+config PKVM_SELFTESTS
+ bool "Protected KVM hypervisor selftests"
+ help
+ Say Y here to enable Protected KVM (pKVM) hypervisor selftests
+ during boot. Failure reports will panic the hypervisor. This is
+ intended for EL2 hypervisor development.
+
+ If unsure, say N.
+
config PROTECTED_NVHE_STACKTRACE
bool "Protected KVM hypervisor stacktraces"
depends on NVHE_EL2_DEBUG
diff --git a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
index 0972faccc2af..a9b2677227cc 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/mem_protect.h
@@ -90,4 +90,10 @@ static __always_inline void __load_host_stage2(void)
else
write_sysreg(0, vttbr_el2);
}
+
+#ifdef CONFIG_PKVM_SELFTESTS
+void pkvm_ownership_selftest(void);
+#else
+static inline void pkvm_ownership_selftest(void) { }
+#endif
#endif /* __KVM_NVHE_MEM_PROTECT__ */
diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
index e75374d682f4..6a01ffe3d117 100644
--- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
+++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
@@ -1306,3 +1306,113 @@ int __pkvm_host_unshare_ffa(u64 pfn, u64 nr_pages)
return ret;
}
+
+#ifdef CONFIG_PKVM_SELFTESTS
+struct pkvm_expected_state {
+ enum pkvm_page_state host;
+ enum pkvm_page_state hyp;
+};
+
+static struct pkvm_expected_state selftest_state;
+static struct hyp_page *selftest_page;
+
+static void assert_page_state(void)
+{
+ void *virt = hyp_page_to_virt(selftest_page);
+ u64 size = PAGE_SIZE << selftest_page->order;
+ u64 phys = hyp_virt_to_phys(virt);
+
+ host_lock_component();
+ WARN_ON(__host_check_page_state_range(phys, size, selftest_state.host));
+ host_unlock_component();
+
+ hyp_lock_component();
+ WARN_ON(__hyp_check_page_state_range((u64)virt, size, selftest_state.hyp));
+ hyp_unlock_component();
+}
+
+#define assert_transition_res(res, fn, ...) \
+ do { \
+ WARN_ON(fn(__VA_ARGS__) != res); \
+ assert_page_state(); \
+ } while (0)
+
+void pkvm_ownership_selftest(void)
+{
+ void *virt = hyp_alloc_pages(&host_s2_pool, 0);
+ u64 phys, size, pfn;
+
+ WARN_ON(!virt);
+ selftest_page = hyp_virt_to_page(virt);
+ selftest_page->refcount = 0;
+
+ size = PAGE_SIZE << selftest_page->order;
+ phys = hyp_virt_to_phys(virt);
+ pfn = hyp_phys_to_pfn(phys);
+
+ selftest_state.host = PKVM_NOPAGE;
+ selftest_state.hyp = PKVM_PAGE_OWNED;
+ assert_page_state();
+ assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_unshare_ffa, pfn, 1);
+ assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
+
+ selftest_state.host = PKVM_PAGE_OWNED;
+ selftest_state.hyp = PKVM_NOPAGE;
+ assert_transition_res(0, __pkvm_hyp_donate_host, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
+ assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
+
+ selftest_state.host = PKVM_PAGE_SHARED_OWNED;
+ selftest_state.hyp = PKVM_PAGE_SHARED_BORROWED;
+ assert_transition_res(0, __pkvm_host_share_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
+
+ assert_transition_res(0, hyp_pin_shared_mem, virt, virt + size);
+ WARN_ON(!hyp_page_count(virt));
+ assert_transition_res(-EBUSY, __pkvm_host_unshare_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
+
+ hyp_unpin_shared_mem(virt, virt + size);
+ assert_page_state();
+ WARN_ON(hyp_page_count(virt));
+ assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
+
+ selftest_state.host = PKVM_PAGE_OWNED;
+ selftest_state.hyp = PKVM_NOPAGE;
+ assert_transition_res(0, __pkvm_host_unshare_hyp, pfn);
+
+ selftest_state.host = PKVM_PAGE_SHARED_OWNED;
+ selftest_state.hyp = PKVM_NOPAGE;
+ assert_transition_res(0, __pkvm_host_share_ffa, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
+ assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn);
+ assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
+ assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
+
+ selftest_state.host = PKVM_PAGE_OWNED;
+ selftest_state.hyp = PKVM_NOPAGE;
+ assert_transition_res(0, __pkvm_host_unshare_ffa, pfn, 1);
+
+ selftest_state.host = PKVM_NOPAGE;
+ selftest_state.hyp = PKVM_PAGE_OWNED;
+ assert_transition_res(0, __pkvm_host_donate_hyp, pfn, 1);
+
+ selftest_page->refcount = 1;
+ hyp_put_page(&host_s2_pool, virt);
+}
+#endif
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index cbdd18cd3f98..d154e80fe6b9 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -306,6 +306,8 @@ void __noreturn __pkvm_init_finalise(void)
goto out;
pkvm_hyp_vm_table_init(vm_table_base);
+
+ pkvm_ownership_selftest();
out:
/*
* We tail-called to here from handle___pkvm_init() and will not return,
--
2.47.0.338.g60cca15819-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] KVM: arm64: Selftest for pKVM transitions
2024-11-29 12:58 [PATCH] KVM: arm64: Selftest for pKVM transitions Quentin Perret
@ 2024-12-20 14:13 ` Will Deacon
2024-12-20 15:14 ` Quentin Perret
0 siblings, 1 reply; 3+ messages in thread
From: Will Deacon @ 2024-12-20 14:13 UTC (permalink / raw)
To: Quentin Perret
Cc: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, linux-arm-kernel, kvmarm,
linux-kernel
On Fri, Nov 29, 2024 at 12:58:00PM +0000, Quentin Perret wrote:
> We have recently found a bug [1] in the pKVM memory ownership
> transitions by code inspection, but it could have been caught with a
> test.
>
> Introduce a boot-time selftest exercising all the known pKVM memory
> transitions and importantly checks the rejection of illegal transitions.
Thanks for doing this!
>
> The new test is hidden behind a new Kconfig option separate from
> CONFIG_EL2_NVHE_DEBUG on purpose as that has side effects on the
> transition checks ([1] doesn't reproduce with EL2 debug enabled).
Hmm. What does a test failure look like without CONFIG_EL2_NVHE_DEBUG
enabled? Do we still get file:line information?
[...]
> +void pkvm_ownership_selftest(void)
> +{
> + void *virt = hyp_alloc_pages(&host_s2_pool, 0);
> + u64 phys, size, pfn;
> +
> + WARN_ON(!virt);
> + selftest_page = hyp_virt_to_page(virt);
> + selftest_page->refcount = 0;
> +
> + size = PAGE_SIZE << selftest_page->order;
> + phys = hyp_virt_to_phys(virt);
> + pfn = hyp_phys_to_pfn(phys);
> +
> + selftest_state.host = PKVM_NOPAGE;
> + selftest_state.hyp = PKVM_PAGE_OWNED;
> + assert_page_state();
> + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_unshare_ffa, pfn, 1);
> + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
> +
> + selftest_state.host = PKVM_PAGE_OWNED;
> + selftest_state.hyp = PKVM_NOPAGE;
> + assert_transition_res(0, __pkvm_hyp_donate_host, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
Should we recheck the unshare transitions here?
> + selftest_state.host = PKVM_PAGE_SHARED_OWNED;
> + selftest_state.hyp = PKVM_PAGE_SHARED_BORROWED;
> + assert_transition_res(0, __pkvm_host_share_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
We could check unshare_ffa here.
> + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> +
> + assert_transition_res(0, hyp_pin_shared_mem, virt, virt + size);
Is it worth trying an extra pin + unpin?
> + WARN_ON(!hyp_page_count(virt));
Can we assert an exact value (e.g. count == 1)?
> + assert_transition_res(-EBUSY, __pkvm_host_unshare_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
unshare_ffa again here.
> + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> +
> + hyp_unpin_shared_mem(virt, virt + size);
> + assert_page_state();
> + WARN_ON(hyp_page_count(virt));
> + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
Is this block actually testing anything new? Once we've asserted the page
state and checked the refcount, the transitions seem redundant to me.
> + selftest_state.host = PKVM_PAGE_OWNED;
> + selftest_state.hyp = PKVM_NOPAGE;
> + assert_transition_res(0, __pkvm_host_unshare_hyp, pfn);
> +
> + selftest_state.host = PKVM_PAGE_SHARED_OWNED;
> + selftest_state.hyp = PKVM_NOPAGE;
> + assert_transition_res(0, __pkvm_host_share_ffa, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn);
> + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
> +
> + selftest_state.host = PKVM_PAGE_OWNED;
> + selftest_state.hyp = PKVM_NOPAGE;
> + assert_transition_res(0, __pkvm_host_unshare_ffa, pfn, 1);
Try it twice?
Will
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] KVM: arm64: Selftest for pKVM transitions
2024-12-20 14:13 ` Will Deacon
@ 2024-12-20 15:14 ` Quentin Perret
0 siblings, 0 replies; 3+ messages in thread
From: Quentin Perret @ 2024-12-20 15:14 UTC (permalink / raw)
To: Will Deacon
Cc: Marc Zyngier, Oliver Upton, Joey Gouly, Suzuki K Poulose,
Zenghui Yu, Catalin Marinas, linux-arm-kernel, kvmarm,
linux-kernel
On Friday 20 Dec 2024 at 14:13:22 (+0000), Will Deacon wrote:
> On Fri, Nov 29, 2024 at 12:58:00PM +0000, Quentin Perret wrote:
> > We have recently found a bug [1] in the pKVM memory ownership
> > transitions by code inspection, but it could have been caught with a
> > test.
> >
> > Introduce a boot-time selftest exercising all the known pKVM memory
> > transitions and importantly checks the rejection of illegal transitions.
>
> Thanks for doing this!
>
> >
> > The new test is hidden behind a new Kconfig option separate from
> > CONFIG_EL2_NVHE_DEBUG on purpose as that has side effects on the
> > transition checks ([1] doesn't reproduce with EL2 debug enabled).
>
> Hmm. What does a test failure look like without CONFIG_EL2_NVHE_DEBUG
> enabled? Do we still get file:line information?
Nope, sadly we don't, but if you get a failure after turning on that
config then you can at least be confident there is a problem with your
code :-)
As discussed in the other thread, once we get the hyp state into the
vmemmap (I've got patches that do exactly that, happy to send them out)
we could just plain remove the 'skip_pgtable_check()' logic, and then
I'm not opposed to merging CONFIG_PKVM_SELFTESTS and CONFIG_EL2_NVHE_DEBUG
together as the latter won't have the same side effects.
> > +void pkvm_ownership_selftest(void)
> > +{
> > + void *virt = hyp_alloc_pages(&host_s2_pool, 0);
> > + u64 phys, size, pfn;
> > +
> > + WARN_ON(!virt);
> > + selftest_page = hyp_virt_to_page(virt);
> > + selftest_page->refcount = 0;
> > +
> > + size = PAGE_SIZE << selftest_page->order;
> > + phys = hyp_virt_to_phys(virt);
> > + pfn = hyp_phys_to_pfn(phys);
> > +
> > + selftest_state.host = PKVM_NOPAGE;
> > + selftest_state.hyp = PKVM_PAGE_OWNED;
> > + assert_page_state();
> > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_unshare_ffa, pfn, 1);
> > + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
> > +
> > + selftest_state.host = PKVM_PAGE_OWNED;
> > + selftest_state.hyp = PKVM_NOPAGE;
> > + assert_transition_res(0, __pkvm_hyp_donate_host, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> > + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
>
> Should we recheck the unshare transitions here?
Yep, sounds like a good idea.
> > + selftest_state.host = PKVM_PAGE_SHARED_OWNED;
> > + selftest_state.hyp = PKVM_PAGE_SHARED_BORROWED;
> > + assert_transition_res(0, __pkvm_host_share_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
>
> We could check unshare_ffa here.
Sadly no, unshare_ffa() isn't super smart, it'll only check that a
page is SHARED_OWNED from the host's PoV and turn it to PAGE_OWNED. So
we'd break the state in this case :/
> > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> > +
> > + assert_transition_res(0, hyp_pin_shared_mem, virt, virt + size);
>
> Is it worth trying an extra pin + unpin?
You mean doing pin > pin > unpin > sanity checks > unpin ?
I guess that would test that we can indeed stack the pins, so sounds
like a good idea too.
> > + WARN_ON(!hyp_page_count(virt));
>
> Can we assert an exact value (e.g. count == 1)?
Sure.
> > + assert_transition_res(-EBUSY, __pkvm_host_unshare_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
>
> unshare_ffa again here.
Same as above.
> > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> > +
> > + hyp_unpin_shared_mem(virt, virt + size);
> > + assert_page_state();
> > + WARN_ON(hyp_page_count(virt));
> > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
>
> Is this block actually testing anything new? Once we've asserted the page
> state and checked the refcount, the transitions seem redundant to me.
The idea was to check if the previous transitions had other sad effects
not visible through the <state, refcount> pair that would cause
problems. But I guess that is a bit far fetched so happy to remove that
part.
> > + selftest_state.host = PKVM_PAGE_OWNED;
> > + selftest_state.hyp = PKVM_NOPAGE;
> > + assert_transition_res(0, __pkvm_host_unshare_hyp, pfn);
> > +
> > + selftest_state.host = PKVM_PAGE_SHARED_OWNED;
> > + selftest_state.hyp = PKVM_NOPAGE;
> > + assert_transition_res(0, __pkvm_host_share_ffa, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_share_ffa, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_donate_hyp, pfn, 1);
> > + assert_transition_res(-EPERM, __pkvm_host_share_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_host_unshare_hyp, pfn);
> > + assert_transition_res(-EPERM, __pkvm_hyp_donate_host, pfn, 1);
> > + assert_transition_res(-EPERM, hyp_pin_shared_mem, virt, virt + size);
> > +
> > + selftest_state.host = PKVM_PAGE_OWNED;
> > + selftest_state.hyp = PKVM_NOPAGE;
> > + assert_transition_res(0, __pkvm_host_unshare_ffa, pfn, 1);
>
> Try it twice?
Sg.
Thanks!
Quentin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-12-20 17:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-29 12:58 [PATCH] KVM: arm64: Selftest for pKVM transitions Quentin Perret
2024-12-20 14:13 ` Will Deacon
2024-12-20 15:14 ` Quentin Perret
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).