Re: [PATCH v6 37/64] KVM: arm64: nv: Restrict S2 RD/WR permissions to match the guest's

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Fri, Jan 28, 2022 at 12:18:45PM +0000, Marc Zyngier wrote:
> When mapping a page in a shadow stage-2, special care must be
> taken not to be more permissive than the guest is (writable or
> readable page when the guest hasn't set that permission).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_nested.h | 15 +++++++++++++++
>  arch/arm64/kvm/mmu.c                | 14 +++++++++++++-
>  arch/arm64/kvm/nested.c             |  2 +-
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_nested.h b/arch/arm64/include/asm/kvm_nested.h
> index 4fad4d3848ce..f4b846d09d86 100644
> --- a/arch/arm64/include/asm/kvm_nested.h
> +++ b/arch/arm64/include/asm/kvm_nested.h
> @@ -97,6 +97,21 @@ static inline u32 kvm_s2_trans_esr(struct kvm_s2_trans *trans)
>  	return trans->esr;
>  }
>  
> +static inline bool kvm_s2_trans_readable(struct kvm_s2_trans *trans)
> +{
> +	return trans->readable;
> +}
> +
> +static inline bool kvm_s2_trans_writable(struct kvm_s2_trans *trans)
> +{
> +	return trans->writable;
> +}
> +
> +static inline bool kvm_s2_trans_executable(struct kvm_s2_trans *trans)
> +{
> +	return !(trans->upper_attr & BIT(54));
> +}
> +
>  extern int kvm_walk_nested_s2(struct kvm_vcpu *vcpu, phys_addr_t gipa,
>  			      struct kvm_s2_trans *result);
>  
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 36f7ecb4f81b..7c56e1522d3c 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1247,6 +1247,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	if (exec_fault && device)
>  		return -ENOEXEC;
>  
> +	/*
> +	 * Potentially reduce shadow S2 permissions to match the guest's own
> +	 * S2. For exec faults, we'd only reach this point if the guest
> +	 * actually allowed it (see kvm_s2_handle_perm_fault).
> +	 */
> +	if (kvm_is_shadow_s2_fault(vcpu)) {
> +		writable &= kvm_s2_trans_writable(nested);

I was a bit confused about writable being true when write_fault is false. After
some digging, it turns out that hva_to_pfn() oportunistically makes writable
true, even for read faults.

> +		if (!kvm_s2_trans_readable(nested))
> +			prot &= ~KVM_PGTABLE_PROT_R;

The local variable "prot" is initialized to KVM_PGTABLE_PROT_R, so this check
makes sense.

> +	}
> +
>  	spin_lock(&kvm->mmu_lock);
>  	pgt = vcpu->arch.hw_mmu->pgt;
>  	if (mmu_notifier_retry(kvm, mmu_seq))
> @@ -1285,7 +1296,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  
>  	if (device)
>  		prot |= KVM_PGTABLE_PROT_DEVICE;
> -	else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC))
> +	else if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC) &&
> +		 kvm_s2_trans_executable(nested))
>  		prot |= KVM_PGTABLE_PROT_X;
>  
>  	/*
> diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c
> index 0a9708f776fc..a74ffb1d2064 100644
> --- a/arch/arm64/kvm/nested.c
> +++ b/arch/arm64/kvm/nested.c
> @@ -481,7 +481,7 @@ int kvm_s2_handle_perm_fault(struct kvm_vcpu *vcpu, struct kvm_s2_trans *trans)
>  		return 0;
>  
>  	if (kvm_vcpu_trap_is_iabt(vcpu)) {
> -		forward_fault = (trans->upper_attr & BIT(54));
> +		forward_fault = !kvm_s2_trans_executable(trans);
>  	} else {
>  		bool write_fault = kvm_is_write_fault(vcpu);

The patch looks good to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex