Re: [RFC v2 4/7] kvm: arm64: introduce check_user

From: Peng Liang <liangpeng10@huawei.com>
To: Andrew Jones <drjones@redhat.com>
Cc: zhang.zhanghailiang@huawei.com, kvm@vger.kernel.org,
	maz@kernel.org, will@kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [RFC v2 4/7] kvm: arm64: introduce check_user
Date: Fri, 18 Sep 2020 17:25:51 +0800	[thread overview]
Message-ID: <20569744-7467-456f-3027-64d71d7c0b0c@huawei.com> (raw)
In-Reply-To: <20200918074135.67ahnd6rlh7db3is@kamzik.brq.redhat.com>

On 9/18/2020 3:41 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 08:00:58PM +0800, Peng Liang wrote:
>> Currently, if we need to check the value of the register defined by user
>> space, we should check it in set_user.  However, some system registers
>> may use the same set_user (for example, almost all ID registers), which
>> make it difficult to validate the value defined by user space.
> 
> If sharing set_user no longer makes sense for ID registers, then we need
> to rework the code so it's no longer shared. As I keep saying, we need
> to address this problem one ID register at a time. So, IMO, the approach
> should be to change one ID register at a time from using ID_SANITISED()
> to having its own table entry with its own set/get_user code. There may
> still be opportunity to share code among the ID registers, in which case
> refactoring can be done as needed too.
> 
> Thanks,
> drew
> 

Thank you for your advise.  Currently, the implementation is a little dirty.
Removing the shared set_user of ID registers should make it clean.  I will
refactor the code to make it in next version.

Thanks,
Peng

>>
>> Introduce check_user to solve the problem.  And apply check_user before
>> set_user to make sure that the value of register is valid.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 7 +++++++
>>  arch/arm64/kvm/sys_regs.h | 6 ++++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 2b0fa8d5ac62..86ebb8093c3c 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -2684,6 +2684,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>>  {
>>  	const struct sys_reg_desc *r;
>>  	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
>> +	int err;
>>  
>>  	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
>>  		return demux_c15_set(reg->id, uaddr);
>> @@ -2699,6 +2700,12 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
>>  	if (sysreg_hidden_from_user(vcpu, r))
>>  		return -ENOENT;
>>  
>> +	if (r->check_user) {
>> +		err = (r->check_user)(vcpu, r, reg, uaddr);
>> +		if (err)
>> +			return err;
>> +	}
>> +
>>  	if (r->set_user)
>>  		return (r->set_user)(vcpu, r, reg, uaddr);
>>  
>> diff --git a/arch/arm64/kvm/sys_regs.h b/arch/arm64/kvm/sys_regs.h
>> index 5a6fc30f5989..9bce5e9a3490 100644
>> --- a/arch/arm64/kvm/sys_regs.h
>> +++ b/arch/arm64/kvm/sys_regs.h
>> @@ -53,6 +53,12 @@ struct sys_reg_desc {
>>  			const struct kvm_one_reg *reg, void __user *uaddr);
>>  	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>>  			const struct kvm_one_reg *reg, void __user *uaddr);
>> +	/*
>> +	 * Check the value userspace passed.  It should return 0 on success and
>> +	 * otherwise on failure.
>> +	 */
>> +	int (*check_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
>> +			  const struct kvm_one_reg *reg, void __user *uaddr);
>>  
>>  	/* Return mask of REG_* runtime visibility overrides */
>>  	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
>> -- 
>> 2.26.2
>>
> 
> .
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm