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
>
> 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
>
