Re: [PATCH kernel 1/4] KVM: PPC: Validate all tces before updating tables

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]

On Thu, Aug 30, 2018 at 01:16:44PM +1000, Alexey Kardashevskiy wrote:
> The KVM TCE handlers are written in a way so they fail when either
> something went horribly wrong or the userspace did some obvious mistake
> such as passing a misaligned address.
> 
> We are going to enhance the TCE checker to fail on attempts to map bigger
> IOMMU page than the underlying pinned memory so let's valitate TCE
> beforehand.
> 
> This should cause no behavioral change.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

With one misgiving..

> ---
>  arch/powerpc/kvm/book3s_64_vio.c    | 8 ++++++++
>  arch/powerpc/kvm/book3s_64_vio_hv.c | 4 ++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index 9a3f264..0fef22b 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -599,6 +599,14 @@ long kvmppc_h_put_tce_indirect(struct kvm_vcpu *vcpu,
>  		ret = kvmppc_tce_validate(stt, tce);
>  		if (ret != H_SUCCESS)
>  			goto unlock_exit;
> +	}
> +
> +	for (i = 0; i < npages; ++i) {
> +		if (get_user(tce, tces + i)) {

This looks unsafe, because we validate, then regrab the TCE from
userspace which could have been changed by another thread.

But it actually is safe, because the relevant checks will be
re-executed in the following code.  If userspace tries to change this
dodgily it will result in a messier failure mode but won't threaten
the host.

Long term, I think we would be better off copying everything into
kernel space then doing the validation just once.  But the difference
should only become apparent with a malicious or badly broken guest,
and in the meantime this series addresses a real problem.

So, I think we should go ahead with it despite that imperfection.


> +			ret = H_TOO_HARD;
> +			goto unlock_exit;
> +		}
> +		tce = be64_to_cpu(tce);
>  
>  		if (kvmppc_gpa_to_ua(vcpu->kvm,
>  				tce & ~(TCE_PCI_READ | TCE_PCI_WRITE),
> diff --git a/arch/powerpc/kvm/book3s_64_vio_hv.c b/arch/powerpc/kvm/book3s_64_vio_hv.c
> index 506a4d4..7ab6f3f 100644
> --- a/arch/powerpc/kvm/book3s_64_vio_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_vio_hv.c
> @@ -501,6 +501,10 @@ long kvmppc_rm_h_put_tce_indirect(struct kvm_vcpu *vcpu,
>  		ret = kvmppc_tce_validate(stt, tce);
>  		if (ret != H_SUCCESS)
>  			goto unlock_exit;
> +	}
> +
> +	for (i = 0; i < npages; ++i) {
> +		unsigned long tce = be64_to_cpu(((u64 *)tces)[i]);
>  
>  		ua = 0;
>  		if (kvmppc_gpa_to_ua(vcpu->kvm,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]