Re: [PATCH 19/26] KVM: arm64: Make struct kvm_regs userspace-only

2020-05-27 Thread Marc Zyngier

On 2020-05-26 17:29, James Morse wrote:

Hi Marc,

On 22/04/2020 13:00, Marc Zyngier wrote:

struct kvm_regs is used by userspace to indicate which register gets
accessed by the {GET,SET}_ONE_REG API. But as we're about to refactor
the layout of the in-kernel register structures, we need the kernel to
move away from it.

Let's make kvm_regs userspace only, and let the kernel map it to its 
own

internal representation.



diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 23ebe51410f06..9fec9231b63e2 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -102,6 +102,55 @@ static int core_reg_size_from_offset(const struct 
kvm_vcpu *vcpu, u64 off)

return size;
 }

+static void *core_reg_addr(struct kvm_vcpu *vcpu, const struct 
kvm_one_reg *reg)

+{
+   u64 off = core_reg_offset_from_id(reg->id);
+
+   switch (off) {



+   default:
+   return NULL;


Doesn't this switch statement catch an out of range offset, and a
misaligned offset?

... We still test for those explicitly in the caller. Better safe than 
implicit?


Indeed, this is not supposed to happen at all. Maybe I should just fold
validate_core_offset offset there, and make this NULL value the error
case.




+   }
+}


With the reset thing reported by Zenghui and Zengtao on the previous
patch fixed:
Reviewed-by: James Morse 

(otherwise struct kvm_regs isn't userspace-only!)


Indeed!

Thanks,

M.
--
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH 19/26] KVM: arm64: Make struct kvm_regs userspace-only

2020-05-26 Thread James Morse
Hi Marc,

On 22/04/2020 13:00, Marc Zyngier wrote:
> struct kvm_regs is used by userspace to indicate which register gets
> accessed by the {GET,SET}_ONE_REG API. But as we're about to refactor
> the layout of the in-kernel register structures, we need the kernel to
> move away from it.
> 
> Let's make kvm_regs userspace only, and let the kernel map it to its own
> internal representation.

> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index 23ebe51410f06..9fec9231b63e2 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -102,6 +102,55 @@ static int core_reg_size_from_offset(const struct 
> kvm_vcpu *vcpu, u64 off)
>   return size;
>  }
>  
> +static void *core_reg_addr(struct kvm_vcpu *vcpu, const struct kvm_one_reg 
> *reg)
> +{
> + u64 off = core_reg_offset_from_id(reg->id);
> +
> + switch (off) {

> + default:
> + return NULL;

Doesn't this switch statement catch an out of range offset, and a misaligned 
offset?

... We still test for those explicitly in the caller. Better safe than implicit?


> + }
> +}

With the reset thing reported by Zenghui and Zengtao on the previous patch 
fixed:
Reviewed-by: James Morse 

(otherwise struct kvm_regs isn't userspace-only!)


Thanks,

James
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm