hypervisor: kvm: aarch64: fix get_device_attr() UB

DeviceFd::get_device_attr should be marked as unsafe, because it
allows writing to an arbitrary address.  I have opened a kvm-ioctls
PR[1] to fix this.  The hypervisor crate was using the function
unsafely by passing it addresses of immutable variables.  I noticed
this because an optimisation change[2] in Rust 1.80.0 caused the
kvm::aarch64::gic::tests::test_get_set_icc_regs test to start failing
when built in release mode.

To fix this, I've broken up the _access functions into _set and _get
variants, with the _get variant using a pointer to a mutable variable.
This has the side effect of making these functions a bit nicer to use,
because the caller now has no need to use references at all, for
either getting or setting.

[1]: rust-vmm/kvm#273
[2]: rust-lang/rust@d2d24e3

Signed-off-by: Alyssa Ross <hi@alyssa.is>
(cherry picked from commit 02f146f)

Author: alyssais

hypervisor/src/kvm/aarch64/gic/dist_regs.rs

@@ -77,46 +77,61 @@ static VGIC_DIST_REGS: &[DistReg] = &[
     VGIC_DIST_REG!(GICD_IPRIORITYR, 8, 0),
 ];
 
-fn dist_attr_access(gic: &DeviceFd, offset: u32, val: &u32, set: bool) -> Result<()> {
-    let mut gic_dist_attr = kvm_device_attr {
+fn dist_attr_set(gic: &DeviceFd, offset: u32, val: u32) -> Result<()> {
+    let gic_dist_attr = kvm_device_attr {
         group: KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
         attr: offset as u64,
-        addr: val as *const u32 as u64,
+        addr: &val as *const u32 as u64,
         flags: 0,
     };
-    if set {
-        gic.set_device_attr(&gic_dist_attr).map_err(|e| {
-            Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into()))
-        })?;
-    } else {
-        gic.get_device_attr(&mut gic_dist_attr).map_err(|e| {
-            Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into()))
-        })?;
-    }
+
+    gic.set_device_attr(&gic_dist_attr).map_err(|e| {
+        Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into()))
+    })?;
+
     Ok(())
 }
 
+fn dist_attr_get(gic: &DeviceFd, offset: u32) -> Result<u32> {
+    let mut val = 0;
+
+    let mut gic_dist_attr = kvm_device_attr {
+        group: KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+        attr: offset as u64,
+        addr: &mut val as *mut u32 as u64,
+        flags: 0,
+    };
+
+    // get_device_attr should be marked as unsafe, and will be in future.
+    // SAFETY: gic_dist_attr.addr is safe to write to.
+    gic.get_device_attr(&mut gic_dist_attr).map_err(|e| {
+        Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into()))
+    })?;
+
+    Ok(val)
+}
+
 /// Get the distributor control register.
 pub fn read_ctlr(gic: &DeviceFd) -> Result<u32> {
-    let val: u32 = 0;
-    dist_attr_access(gic, GICD_CTLR, &val, false)?;
-    Ok(val)
+    dist_attr_get(gic, GICD_CTLR)
 }
 
 /// Set the distributor control register.
 pub fn write_ctlr(gic: &DeviceFd, val: u32) -> Result<()> {
-    dist_attr_access(gic, GICD_CTLR, &val, true)
+    dist_attr_set(gic, GICD_CTLR, val)
 }
 
 fn get_interrupts_num(gic: &DeviceFd) -> Result<u32> {
-    let num_irq = 0;
+    let mut num_irq = 0;
 
     let mut nr_irqs_attr = kvm_device_attr {
         group: KVM_DEV_ARM_VGIC_GRP_NR_IRQS,
         attr: 0,
-        addr: &num_irq as *const u32 as u64,
+        addr: &mut num_irq as *mut u32 as u64,
         flags: 0,
     };
+    // get_device_attr should be marked as unsafe, and will be in future.
+    // SAFETY: nr_irqs_attr.addr is safe to write to.
     gic.get_device_attr(&mut nr_irqs_attr).map_err(|e| {
         Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into()))
     })?;
@@ -158,8 +173,7 @@ pub fn set_dist_regs(gic: &DeviceFd, state: &[u32]) -> Result<()> {
         let end = compute_reg_len(gic, dreg, base)?;
 
         while base < end {
-            let val = state[idx];
-            dist_attr_access(gic, base, &val, true)?;
+            dist_attr_set(gic, base, state[idx])?;
             idx += 1;
             base += REG_SIZE as u32;
         }
@@ -175,9 +189,7 @@ pub fn get_dist_regs(gic: &DeviceFd) -> Result<Vec<u32>> {
         let end = compute_reg_len(gic, dreg, base)?;
 
         while base < end {
-            let val: u32 = 0;
-            dist_attr_access(gic, base, &val, false)?;
-            state.push(val);
+            state.push(dist_attr_get(gic, base)?);
             base += REG_SIZE as u32;
         }
     }

hypervisor/src/kvm/aarch64/gic/icc_regs.rs

@@ -79,25 +79,40 @@ static VGIC_ICC_REGS: &[u64] = &[
     SYS_ICC_AP1R3_EL1,
 ];
 
-fn icc_attr_access(gic: &DeviceFd, offset: u64, typer: u64, val: &u32, set: bool) -> Result<()> {
-    let mut gic_icc_attr = kvm_device_attr {
+fn icc_attr_set(gic: &DeviceFd, offset: u64, typer: u64, val: u32) -> Result<()> {
+    let gic_icc_attr = kvm_device_attr {
         group: KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
         attr: ((typer & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) | offset), // this needs the mpidr
-        addr: val as *const u32 as u64,
+        addr: &val as *const u32 as u64,
         flags: 0,
     };
-    if set {
-        gic.set_device_attr(&gic_icc_attr).map_err(|e| {
-            Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into()))
-        })?;
-    } else {
-        gic.get_device_attr(&mut gic_icc_attr).map_err(|e| {
-            Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into()))
-        })?;
-    }
+
+    gic.set_device_attr(&gic_icc_attr).map_err(|e| {
+        Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into()))
+    })?;
+
     Ok(())
 }
 
+fn icc_attr_get(gic: &DeviceFd, offset: u64, typer: u64) -> Result<u32> {
+    let mut val = 0;
+
+    let mut gic_icc_attr = kvm_device_attr {
+        group: KVM_DEV_ARM_VGIC_GRP_CPU_SYSREGS,
+        attr: ((typer & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) | offset), // this needs the mpidr
+        addr: &mut val as *mut u32 as u64,
+        flags: 0,
+    };
+
+    // get_device_attr should be marked as unsafe, and will be in future.
+    // SAFETY: gic_icc_attr.addr is safe to write to.
+    gic.get_device_attr(&mut gic_icc_attr).map_err(|e| {
+        Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into()))
+    })?;
+
+    Ok(val)
+}
+
 /// Get ICC registers.
 pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result<Vec<u32>> {
     let mut state: Vec<u32> = Vec::new();
@@ -107,10 +122,9 @@ pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result<Vec<u32>> {
     for ix in gicr_typer {
         let i = *ix;
         for icc_offset in VGIC_ICC_REGS {
-            let val = 0;
             if *icc_offset == SYS_ICC_CTLR_EL1 {
                 // calculate priority bits by reading the ctrl_el1 register.
-                icc_attr_access(gic, *icc_offset, i, &val, false)?;
+                let val = icc_attr_get(gic, *icc_offset, i)?;
                 // The priority bits are found in the ICC_CTLR_EL1 register (bits from  10:8).
                 // See page 194 from https://static.docs.arm.com/ihi0069/c/IHI0069C_gic_
                 // architecture_specification.pdf.
@@ -130,21 +144,18 @@ pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result<Vec<u32>> {
             // 7 bits of priority.
             else if *icc_offset == SYS_ICC_AP0R1_EL1 || *icc_offset == SYS_ICC_AP1R1_EL1 {
                 if num_priority_bits >= 6 {
-                    icc_attr_access(gic, *icc_offset, i, &val, false)?;
-                    state.push(val);
+                    state.push(icc_attr_get(gic, *icc_offset, i)?);
                 }
             } else if *icc_offset == SYS_ICC_AP0R2_EL1
                 || *icc_offset == SYS_ICC_AP0R3_EL1
                 || *icc_offset == SYS_ICC_AP1R2_EL1
                 || *icc_offset == SYS_ICC_AP1R3_EL1
             {
                 if num_priority_bits == 7 {
-                    icc_attr_access(gic, *icc_offset, i, &val, false)?;
-                    state.push(val);
+                    state.push(icc_attr_get(gic, *icc_offset, i)?);
                 }
             } else {
-                icc_attr_access(gic, *icc_offset, i, &val, false)?;
-                state.push(val);
+                state.push(icc_attr_get(gic, *icc_offset, i)?);
             }
         }
     }
@@ -165,7 +176,7 @@ pub fn set_icc_regs(gic: &DeviceFd, gicr_typer: &[u64], state: &[u32]) -> Result
             }
             if *icc_offset == SYS_ICC_AP0R1_EL1 || *icc_offset == SYS_ICC_AP1R1_EL1 {
                 if num_priority_bits >= 6 {
-                    icc_attr_access(gic, *icc_offset, i, &state[idx], true)?;
+                    icc_attr_set(gic, *icc_offset, i, state[idx])?;
                     idx += 1;
                 }
                 continue;
@@ -176,12 +187,12 @@ pub fn set_icc_regs(gic: &DeviceFd, gicr_typer: &[u64], state: &[u32]) -> Result
                 || *icc_offset == SYS_ICC_AP1R3_EL1
             {
                 if num_priority_bits == 7 {
-                    icc_attr_access(gic, *icc_offset, i, &state[idx], true)?;
+                    icc_attr_set(gic, *icc_offset, i, state[idx])?;
                     idx += 1;
                 }
                 continue;
             }
-            icc_attr_access(gic, *icc_offset, i, &state[idx], true)?;
+            icc_attr_set(gic, *icc_offset, i, state[idx])?;
             idx += 1;
         }
     }