On Mon, Jun 03, 2019 at 05:52:07PM +0100, Dave Martin wrote:
> Since commit d26c25a9d19b ("arm64: KVM: Tighten guest core register
> access from userspace"), KVM_{GET,SET}_ONE_REG rejects register IDs
> that do not correspond to a single underlying architectural register.
> 
> KVM_GET_REG_LIST was not changed to match however: instead, it
> simply yields a list of 32-bit register IDs that together cover the
> whole kvm_regs struct.  This means that if userspace tries to use
> the resulting list of IDs directly to drive calls to KVM_*_ONE_REG,
> some of those calls will now fail.
> 
> This was not the intention.  Instead, iterating KVM_*_ONE_REG over
> the list of IDs returned by KVM_GET_REG_LIST should be guaranteed
> to work.
> 
> This patch fixes the problem by splitting validate_core_offset()
> into a backend core_reg_size_from_offset() which does all of the
> work except for checking that the size field in the register ID
> matches, and kvm_arm_copy_reg_indices() and num_core_regs() are
> converted to use this to enumerate the valid offsets.
> 
> kvm_arm_copy_reg_indices() now also sets the register ID size field
> appropriately based on the value returned, so the register ID
> supplied to userspace is fully qualified for use with the register
> access ioctls.

Ah yes, I've seen this issue, but hadn't gotten around to fixing it.

> 
> Cc: sta...@vger.kernel.org
> Fixes: d26c25a9d19b ("arm64: KVM: Tighten guest core register access from 
> userspace")
> Signed-off-by: Dave Martin <dave.mar...@arm.com>
> 
> ---
> 
> Changes since v3:

Hmm, I didn't see a v1-v3.

> 
>  * Rebased onto v5.2-rc1.
> 
>  * Tested with qemu by migrating from one qemu instance to another on
>    ThunderX2.

One of the reasons I was slow to fix this is because QEMU doesn't care
about the core registers when it uses KVM_GET_REG_LIST. It just completely
skips all core reg indices, so it never finds out that they're invalid.
And kvmtool doesn't use KVM_GET_REG_LIST at all. But it's certainly good
to fix this.

> 
> ---
>  arch/arm64/kvm/guest.c | 53 
> +++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 3ae2f82..6527c76 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -70,10 +70,8 @@ static u64 core_reg_offset_from_id(u64 id)
>       return id & ~(KVM_REG_ARCH_MASK | KVM_REG_SIZE_MASK | KVM_REG_ARM_CORE);
>  }
>  
> -static int validate_core_offset(const struct kvm_vcpu *vcpu,
> -                             const struct kvm_one_reg *reg)
> +static int core_reg_size_from_offset(const struct kvm_vcpu *vcpu, u64 off)
>  {
> -     u64 off = core_reg_offset_from_id(reg->id);
>       int size;
>  
>       switch (off) {
> @@ -103,8 +101,7 @@ static int validate_core_offset(const struct kvm_vcpu 
> *vcpu,
>               return -EINVAL;
>       }
>  
> -     if (KVM_REG_SIZE(reg->id) != size ||
> -         !IS_ALIGNED(off, size / sizeof(__u32)))
> +     if (!IS_ALIGNED(off, size / sizeof(__u32)))
>               return -EINVAL;
>  
>       /*
> @@ -115,6 +112,21 @@ static int validate_core_offset(const struct kvm_vcpu 
> *vcpu,
>       if (vcpu_has_sve(vcpu) && core_reg_offset_is_vreg(off))
>               return -EINVAL;
>  
> +     return size;
> +}
> +
> +static int validate_core_offset(const struct kvm_vcpu *vcpu,
> +                             const struct kvm_one_reg *reg)
> +{
> +     u64 off = core_reg_offset_from_id(reg->id);
> +     int size = core_reg_size_from_offset(vcpu, off);
> +
> +     if (size < 0)
> +             return -EINVAL;
> +
> +     if (KVM_REG_SIZE(reg->id) != size)
> +             return -EINVAL;
> +
>       return 0;
>  }
>  
> @@ -453,19 +465,34 @@ static int copy_core_reg_indices(const struct kvm_vcpu 
> *vcpu,
>  {
>       unsigned int i;
>       int n = 0;
> -     const u64 core_reg = KVM_REG_ARM64 | KVM_REG_SIZE_U64 | 
> KVM_REG_ARM_CORE;
>  
>       for (i = 0; i < sizeof(struct kvm_regs) / sizeof(__u32); i++) {
> -             /*
> -              * The KVM_REG_ARM64_SVE regs must be used instead of
> -              * KVM_REG_ARM_CORE for accessing the FPSIMD V-registers on
> -              * SVE-enabled vcpus:
> -              */
> -             if (vcpu_has_sve(vcpu) && core_reg_offset_is_vreg(i))
> +             u64 reg = KVM_REG_ARM64 | KVM_REG_ARM_CORE | i;
> +             int size = core_reg_size_from_offset(vcpu, i);
> +
> +             if (size < 0)
> +                     continue;
> +
> +             switch (size) {
> +             case sizeof(__u32):
> +                     reg |= KVM_REG_SIZE_U32;
> +                     break;
> +
> +             case sizeof(__u64):
> +                     reg |= KVM_REG_SIZE_U64;
> +                     break;
> +
> +             case sizeof(__uint128_t):
> +                     reg |= KVM_REG_SIZE_U128;
> +                     break;
> +
> +             default:
> +                     WARN_ON(1);
>                       continue;
> +             }
>  
>               if (uindices) {
> -                     if (put_user(core_reg | i, uindices))
> +                     if (put_user(reg, uindices))
>                               return -EFAULT;
>                       uindices++;
>               }
> -- 
> 2.1.4
>

Reviewed-by: Andrew Jones <drjo...@redhat.com>

I've also tested this using a kvm selftests test I wrote. I haven't posted
that test yet because it needs some cleanup and I planned on getting back
to that when getting back to fixing this issue. Anyway, before this patch
every other 64-bit core reg index is invalid (because its indexing 32-bits
but claiming a size of 64), all fp regs are invalid, and we were even
providing a couple indices that mapped to struct padding. After this patch
everything is right with the world.

Tested-by: Andrew Jones <drjo...@redhat.com>

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reply via email to