diff mbox series

[v3,02/19] KVM: arm64: Create a helper to check if IPA is valid

Message ID 20220223041844.3984439-3-oupton@google.com
State Not Applicable
Headers show
Series KVM: arm64: Implement PSCI SYSTEM_SUSPEND | expand

Commit Message

Oliver Upton Feb. 23, 2022, 4:18 a.m. UTC 
Create a helper that tests if a given IPA fits within the guest's
address space.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/arm64/include/asm/kvm_mmu.h      | 9 +++++++++
 arch/arm64/kvm/vgic/vgic-kvm-device.c | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Reiji Watanabe Feb. 24, 2022, 6:32 a.m. UTC | #1 
On Tue, Feb 22, 2022 at 8:19 PM Oliver Upton <oupton@google.com> wrote:
>
> Create a helper that tests if a given IPA fits within the guest's
> address space.
>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_mmu.h      | 9 +++++++++
>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 81839e9a8a24..78e8be7ea627 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -111,6 +111,7 @@ alternative_cb_end
>  #else
>
>  #include <linux/pgtable.h>
> +#include <linux/kvm_host.h>
>  #include <asm/pgalloc.h>
>  #include <asm/cache.h>
>  #include <asm/cacheflush.h>
> @@ -147,6 +148,14 @@ static __always_inline unsigned long __kern_hyp_va(unsigned long v)
>  #define kvm_phys_size(kvm)             (_AC(1, ULL) << kvm_phys_shift(kvm))
>  #define kvm_phys_mask(kvm)             (kvm_phys_size(kvm) - _AC(1, ULL))
>
> +/*
> + * Returns true if the provided IPA exists within the VM's IPA space.
> + */
> +static inline bool kvm_ipa_valid(struct kvm *kvm, phys_addr_t guest_ipa)
> +{
> +       return !(guest_ipa & ~kvm_phys_mask(kvm));
> +}
> +
>  #include <asm/kvm_pgtable.h>
>  #include <asm/stage2_pgtable.h>
>
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index c6d52a1fd9c8..e3853a75cb00 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -27,7 +27,7 @@ int vgic_check_iorange(struct kvm *kvm, phys_addr_t ioaddr,
>         if (addr + size < addr)
>                 return -EINVAL;
>
> -       if (addr & ~kvm_phys_mask(kvm) || addr + size > kvm_phys_size(kvm))
> +       if (!kvm_ipa_valid(kvm, addr) || addr + size > kvm_phys_size(kvm))
>                 return -E2BIG;
>
>         return 0;

Reviewed-by: Reiji Watanabe <reijiw@google.com>

It looks like we can use the helper for kvm_handle_guest_abort()
in arch/arm64/kvm/mmu.c as well though.
----------
<...>
        /* Userspace should not be able to register out-of-bounds IPAs */
        VM_BUG_ON(fault_ipa >= kvm_phys_size(vcpu->kvm));
<...>
----------

Thanks,
Reiji
Marc Zyngier Feb. 24, 2022, 12:06 p.m. UTC | #2 
On Wed, 23 Feb 2022 04:18:27 +0000,
Oliver Upton <oupton@google.com> wrote:
> 
> Create a helper that tests if a given IPA fits within the guest's
> address space.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  arch/arm64/include/asm/kvm_mmu.h      | 9 +++++++++
>  arch/arm64/kvm/vgic/vgic-kvm-device.c | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> index 81839e9a8a24..78e8be7ea627 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -111,6 +111,7 @@ alternative_cb_end
>  #else
>  
>  #include <linux/pgtable.h>
> +#include <linux/kvm_host.h>

I'd rather you avoid that. This sort of linux->asm->linux transitive
inclusions always lead to a terrible mess at some point. Which is why
we use #defines below. And yes, the pgtable.h inclusion is a bad
precedent.

>  #include <asm/pgalloc.h>
>  #include <asm/cache.h>
>  #include <asm/cacheflush.h>
> @@ -147,6 +148,14 @@ static __always_inline unsigned long __kern_hyp_va(unsigned long v)
>  #define kvm_phys_size(kvm)		(_AC(1, ULL) << kvm_phys_shift(kvm))
>  #define kvm_phys_mask(kvm)		(kvm_phys_size(kvm) - _AC(1, ULL))
>  
> +/*
> + * Returns true if the provided IPA exists within the VM's IPA space.
> + */
> +static inline bool kvm_ipa_valid(struct kvm *kvm, phys_addr_t guest_ipa)
> +{
> +	return !(guest_ipa & ~kvm_phys_mask(kvm));
> +}
> +

I'm all for the helper, but just make it a #define to be consistent
with the rest of the code.

>  #include <asm/kvm_pgtable.h>
>  #include <asm/stage2_pgtable.h>
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> index c6d52a1fd9c8..e3853a75cb00 100644
> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
> @@ -27,7 +27,7 @@ int vgic_check_iorange(struct kvm *kvm, phys_addr_t ioaddr,
>  	if (addr + size < addr)
>  		return -EINVAL;
>  
> -	if (addr & ~kvm_phys_mask(kvm) || addr + size > kvm_phys_size(kvm))
> +	if (!kvm_ipa_valid(kvm, addr) || addr + size > kvm_phys_size(kvm))
>  		return -E2BIG;

I think you can pretty much use this helper everywhere something is
compared to kvm_phys_size(), and the above becomes:

 if (!kvm_ipa_valid(kvm, addr) || !kvm_ipa_valid(kvm, addr + size - 1))

Same this goes for the couple of occurrences in arch/arm64/kvm/mmu.c.

Thanks,

	M.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 81839e9a8a24..78e8be7ea627 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -111,6 +111,7 @@  alternative_cb_end
 #else
 
 #include <linux/pgtable.h>
+#include <linux/kvm_host.h>
 #include <asm/pgalloc.h>
 #include <asm/cache.h>
 #include <asm/cacheflush.h>
@@ -147,6 +148,14 @@  static __always_inline unsigned long __kern_hyp_va(unsigned long v)
 #define kvm_phys_size(kvm)		(_AC(1, ULL) << kvm_phys_shift(kvm))
 #define kvm_phys_mask(kvm)		(kvm_phys_size(kvm) - _AC(1, ULL))
 
+/*
+ * Returns true if the provided IPA exists within the VM's IPA space.
+ */
+static inline bool kvm_ipa_valid(struct kvm *kvm, phys_addr_t guest_ipa)
+{
+	return !(guest_ipa & ~kvm_phys_mask(kvm));
+}
+
 #include <asm/kvm_pgtable.h>
 #include <asm/stage2_pgtable.h>
 
diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index c6d52a1fd9c8..e3853a75cb00 100644
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -27,7 +27,7 @@  int vgic_check_iorange(struct kvm *kvm, phys_addr_t ioaddr,
 	if (addr + size < addr)
 		return -EINVAL;
 
-	if (addr & ~kvm_phys_mask(kvm) || addr + size > kvm_phys_size(kvm))
+	if (!kvm_ipa_valid(kvm, addr) || addr + size > kvm_phys_size(kvm))
 		return -E2BIG;
 
 	return 0;