Re: [PATCH] KVM: arm64: Selftest for pKVM transitions

[Quentin Perret <qperret@google.com>]

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