| Message ID | 20241128154406.602875-1-qperret@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Always check the state from hyp_ack_unshare() | expand |
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> 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. Hopefully nobody smart tries to add it back here! Will
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
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; - return __hyp_check_page_state_range(addr, size, PKVM_PAGE_SHARED_BORROWED); }
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(-)