KVM: arm64: vgic-v3: Consolidate userspace access for MMIO registers

For userspace accesses to GICv3 MMIO registers (and related data),
vgic_v3_{get,set}_attr are littered with {get,put}_user() calls,
making it hard to audit and reason about.

Consolidate all userspace accesses in vgic_v3_attr_regs_access(),
making the code far simpler to audit.

Reviewed-by: Reiji Watanabe <reijiw@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>

diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c
index f02294b..e9db679 100644 (file)
--- a/arch/arm64/kvm/vgic/vgic-kvm-device.c
+++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c
@@ -512,18 +512,18 @@ int vgic_v3_parse_attr(struct kvm_device *dev, struct kvm_device_attr *attr,
  *
  * @dev:      kvm device handle
  * @attr:     kvm device attribute
- * @reg:      address the value is read or written, NULL for sysregs
  * @is_write: true if userspace is writing a register
  */
 static int vgic_v3_attr_regs_access(struct kvm_device *dev,
                                    struct kvm_device_attr *attr,
-                                   u64 *reg, bool is_write)
+                                   bool is_write)
 {
        struct vgic_reg_attr reg_attr;
        gpa_t addr;
        struct kvm_vcpu *vcpu;
+       bool uaccess;
+       u32 val;
        int ret;
-       u32 tmp32;
 
        ret = vgic_v3_parse_attr(dev, attr, &reg_attr);
        if (ret)
@@ -532,6 +532,21 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
        vcpu = reg_attr.vcpu;
        addr = reg_attr.addr;
 
+       switch (attr->group) {
+       case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
+               /* Sysregs uaccess is performed by the sysreg handling code */
+               uaccess = false;
+               break;
+       default:
+               uaccess = true;
+       }
+
+       if (uaccess && is_write) {
+               u32 __user *uaddr = (u32 __user *)(unsigned long)attr->addr;
+               if (get_user(val, uaddr))
+                       return -EFAULT;
+       }
+
        mutex_lock(&dev->kvm->lock);
 
        if (unlikely(!vgic_initialized(dev->kvm))) {
@@ -546,20 +561,10 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
 
        switch (attr->group) {
        case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-               if (is_write)
-                       tmp32 = *reg;
-
-               ret = vgic_v3_dist_uaccess(vcpu, is_write, addr, &tmp32);
-               if (!is_write)
-                       *reg = tmp32;
+               ret = vgic_v3_dist_uaccess(vcpu, is_write, addr, &val);
                break;
        case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
-               if (is_write)
-                       tmp32 = *reg;
-
-               ret = vgic_v3_redist_uaccess(vcpu, is_write, addr, &tmp32);
-               if (!is_write)
-                       *reg = tmp32;
+               ret = vgic_v3_redist_uaccess(vcpu, is_write, addr, &val);
                break;
        case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
                ret = vgic_v3_cpu_sysregs_uaccess(vcpu, attr, is_write);
@@ -570,14 +575,10 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
                info = (attr->attr & KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_MASK) >>
                        KVM_DEV_ARM_VGIC_LINE_LEVEL_INFO_SHIFT;
                if (info == VGIC_LEVEL_INFO_LINE_LEVEL) {
-                       if (is_write)
-                               tmp32 = *reg;
                        intid = attr->attr &
                                KVM_DEV_ARM_VGIC_LINE_LEVEL_INTID_MASK;
                        ret = vgic_v3_line_level_info_uaccess(vcpu, is_write,
-                                                             intid, &tmp32);
-                       if (!is_write)
-                               *reg = tmp32;
+                                                             intid, &val);
                } else {
                        ret = -EINVAL;
                }
@@ -591,6 +592,12 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
        unlock_all_vcpus(dev->kvm);
 out:
        mutex_unlock(&dev->kvm->lock);
+
+       if (!ret && uaccess && !is_write) {
+               u32 __user *uaddr = (u32 __user *)(unsigned long)attr->addr;
+               ret = put_user(val, uaddr);
+       }
+
        return ret;
 }
 
@@ -605,30 +612,12 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
 
        switch (attr->group) {
        case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-       case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
-               u32 __user *uaddr = (u32 __user *)(long)attr->addr;
-               u32 tmp32;
-               u64 reg;
-
-               if (get_user(tmp32, uaddr))
-                       return -EFAULT;
-
-               reg = tmp32;
-               return vgic_v3_attr_regs_access(dev, attr, &reg, true);
-       }
+       case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+               return vgic_v3_attr_regs_access(dev, attr, true);
        case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
-               return vgic_v3_attr_regs_access(dev, attr, NULL, true);
-       case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
-               u32 __user *uaddr = (u32 __user *)(long)attr->addr;
-               u64 reg;
-               u32 tmp32;
-
-               if (get_user(tmp32, uaddr))
-                       return -EFAULT;
-
-               reg = tmp32;
-               return vgic_v3_attr_regs_access(dev, attr, &reg, true);
-       }
+               return vgic_v3_attr_regs_access(dev, attr, true);
+       case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:
+               return vgic_v3_attr_regs_access(dev, attr, true);
        case KVM_DEV_ARM_VGIC_GRP_CTRL: {
                int ret;
 
@@ -662,30 +651,12 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
 
        switch (attr->group) {
        case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
-       case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS: {
-               u32 __user *uaddr = (u32 __user *)(long)attr->addr;
-               u64 reg;
-               u32 tmp32;
-
-               ret = vgic_v3_attr_regs_access(dev, attr, &reg, false);
-               if (ret)
-                       return ret;
-               tmp32 = reg;
-               return put_user(tmp32, uaddr);
-       }
+       case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
+               return vgic_v3_attr_regs_access(dev, attr, false);
        case KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS:
-               return vgic_v3_attr_regs_access(dev, attr, NULL, false);
-       case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO: {
-               u32 __user *uaddr = (u32 __user *)(long)attr->addr;
-               u64 reg;
-               u32 tmp32;
-
-               ret = vgic_v3_attr_regs_access(dev, attr, &reg, false);
-               if (ret)
-                       return ret;
-               tmp32 = reg;
-               return put_user(tmp32, uaddr);
-       }
+               return vgic_v3_attr_regs_access(dev, attr, false);
+       case KVM_DEV_ARM_VGIC_GRP_LEVEL_INFO:
+               return vgic_v3_attr_regs_access(dev, attr, false);
        }
        return -ENXIO;
 }