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@xxxxxxxxxx>
>> Signed-off-by: Peng Liang <liangpeng10@xxxxxxxxxx>
>> ---
>> 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
>>
>
> .
>
