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;
-
 	return __hyp_check_page_state_range(addr, size,
 					    PKVM_PAGE_SHARED_BORROWED);
 }
-- 
2.47.0.338.g60cca15819-goog

On Thu, 28 Nov 2024 15:44:06 +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.
> 
> [...]

Sorry for letting this one slip.

Applied to fixes, thanks!

[1/1] KVM: arm64: Always check the state from hyp_ack_unshare()
      https://git.kernel.org/kvmarm/kvmarm/c/985bb51f17ab

--
Best,
Oliver

On Thursday 28 Nov 2024 at 15:44:06 (+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;
> -
>  	return __hyp_check_page_state_range(addr, size,
>  					    PKVM_PAGE_SHARED_BORROWED);
>  }
> -- 
> 2.47.0.338.g60cca15819-goog

Shameless inbox bump for this one :-)

It should hopefully be a fairly straightforward fix.

Thanks,
Quentin

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