Messages in this thread Patch in this message |  | | Date | Fri, 29 Nov 2024 13:10:41 +0000 | | From | Quentin Perret <> | | Subject | Re: [PATCH] KVM: arm64: Always check the state from hyp_ack_unshare() |
| |
On Friday 29 Nov 2024 at 09:58:13 (+0000), Will Deacon wrote: > On Thu, Nov 28, 2024 at 03:44:06PM +0000, Quentin Perret wrote: > > There are multiple pKVM memory transitions where the state of a page is > > not cross-checked from the completer's PoV for performance reasons. > > For example, if a page is PKVM_PAGE_OWNED from the initiator's PoV, > > we should be guaranteed by construction that it is PKVM_NOPAGE for > > everybody else, hence allowing us to save a page-table lookup. > > > > When it was introduced, hyp_ack_unshare() followed that logic and bailed > > out without checking the PKVM_PAGE_SHARED_BORROWED state in the > > hypervisor's stage-1. This was correct as we could safely assume that > > all host-initiated shares were directed at the hypervisor at the time. > > But with the introduction of other types of shares (e.g. for FF-A or > > non-protected guests), it is now very much required to cross check this > > state to prevent the host from running __pkvm_host_unshare_hyp() on a > > page shared with TZ or a non-protected guest. > > > > Thankfully, if an attacker were to try this, the hyp_unmap() call from > > hyp_complete_unshare() would fail, hence causing to WARN() from > > __do_unshare() with the host lock held, which is fatal. But this is > > fragile at best, and can hardly be considered a security measure. > > > > Let's just do the right thing and always check the state from > > hyp_ack_unshare(). > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/mem_protect.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > index caba3e4bd09e..e75374d682f4 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c > > @@ -783,9 +783,6 @@ static int hyp_ack_unshare(u64 addr, const struct pkvm_mem_transition *tx) > > if (tx->initiator.id == PKVM_ID_HOST && hyp_page_count((void *)addr)) > > return -EBUSY; > > > > - if (__hyp_ack_skip_pgtable_check(tx)) > > - return 0; > > - > > Acked-by: Will Deacon <will@kernel.org>
Cheers.
> I suppose __hyp_ack_skip_pgtable_check() is now quite poorly named, > since we only want to use it in cases where the page is PKVM_PAGE_OWNED > by the initiator.
I don't mind the name personally, but happy to respin if someone can come up with a better one :-).
> Hopefully nobody smart tries to add it back here!
Right, so here's a patch adding a selftest for this stuff:
https://lore.kernel.org/kvmarm/20241129125800.992468-1-qperret@google.com/
That should help catch future regressions in that area.
FTR, I've started hating on the skip_pgtable_check() logic altogether as enabling CONFIG_EL2_NVHE_DEBUG happens to 'solve' the problem -- it's not exactly intuitive that enabling debug options improves security. The np-guest series moves the host state to the hyp vmemmap, so we can probably nuke __host_ack_skip_pgtable_check() with that as the check becomes really cheap. And we could surely do the same thing for the hyp state, and just always do the cross-check. I'll give it a spin. Thanks, Quentin
|  |