From 02f146fef81c4aa4a7ef3555c176d3b533158d7a Mon Sep 17 00:00:00 2001 From: Alyssa Ross Date: Mon, 12 Aug 2024 17:06:00 +0200 Subject: [PATCH] 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]: https://github.com/rust-vmm/kvm-ioctls/pull/273 [2]: https://github.com/rust-lang/rust/commit/d2d24e395a1e4fcee62ca17bf4cbddb1f903af97 Signed-off-by: Alyssa Ross --- hypervisor/src/kvm/aarch64/gic/dist_regs.rs | 58 +++++---- hypervisor/src/kvm/aarch64/gic/icc_regs.rs | 57 +++++---- hypervisor/src/kvm/aarch64/gic/mod.rs | 111 ++++++++---------- hypervisor/src/kvm/aarch64/gic/redist_regs.rs | 46 +++++--- 4 files changed, 144 insertions(+), 128 deletions(-) diff --git a/hypervisor/src/kvm/aarch64/gic/dist_regs.rs b/hypervisor/src/kvm/aarch64/gic/dist_regs.rs index 0c1a9788b..2da802032 100644 --- a/hypervisor/src/kvm/aarch64/gic/dist_regs.rs +++ b/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<()> { +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, + flags: 0, + }; + + 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 { + 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: val as *const u32 as u64, + addr: &mut val as *mut 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())) - })?; - } - Ok(()) + + // 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 { - 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 { - 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> { 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; } } diff --git a/hypervisor/src/kvm/aarch64/gic/icc_regs.rs b/hypervisor/src/kvm/aarch64/gic/icc_regs.rs index 8124ed681..a72db6c02 100644 --- a/hypervisor/src/kvm/aarch64/gic/icc_regs.rs +++ b/hypervisor/src/kvm/aarch64/gic/icc_regs.rs @@ -79,23 +79,38 @@ static VGIC_ICC_REGS: &[u64] = &[ SYS_ICC_AP1R3_EL1, ]; -fn icc_attr_access(gic: &DeviceFd, offset: u64, typer: u64, val: &u32, set: bool) -> Result<()> { +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, + flags: 0, + }; + + 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 { + 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: val as *const u32 as u64, + addr: &mut val as *mut 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())) - })?; - } - Ok(()) + + // 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. @@ -107,10 +122,9 @@ pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result> { 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,8 +144,7 @@ pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result> { // 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 @@ -139,12 +152,10 @@ pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result> { || *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; } } diff --git a/hypervisor/src/kvm/aarch64/gic/mod.rs b/hypervisor/src/kvm/aarch64/gic/mod.rs index 563b0864d..28d67a3e3 100644 --- a/hypervisor/src/kvm/aarch64/gic/mod.rs +++ b/hypervisor/src/kvm/aarch64/gic/mod.rs @@ -24,34 +24,38 @@ const GITS_CWRITER: u32 = 0x0088; const GITS_CREADR: u32 = 0x0090; const GITS_BASER: u32 = 0x0100; -/// Access an ITS device attribute. -/// -/// This is a helper function to get/set the ITS device attribute depending -/// the bool parameter `set` provided. -pub fn gicv3_its_attr_access( - its_device: &DeviceFd, - group: u32, - attr: u32, - val: &u64, - set: bool, -) -> Result<()> { +fn gicv3_its_attr_set(its_device: &DeviceFd, group: u32, attr: u32, val: u64) -> Result<()> { + let gicv3_its_attr = kvm_bindings::kvm_device_attr { + group, + attr: attr as u64, + addr: &val as *const u64 as u64, + flags: 0, + }; + + its_device + .set_device_attr(&gicv3_its_attr) + .map_err(|e| Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into()))) +} + +fn gicv3_its_attr_get(its_device: &DeviceFd, group: u32, attr: u32) -> Result { + let mut val = 0; + let mut gicv3_its_attr = kvm_bindings::kvm_device_attr { group, attr: attr as u64, - addr: val as *const u64 as u64, + addr: &mut val as *mut u64 as u64, flags: 0, }; - if set { - its_device.set_device_attr(&gicv3_its_attr).map_err(|e| { - Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into())) - }) - } else { - its_device - .get_device_attr(&mut gicv3_its_attr) - .map_err(|e| { - Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into())) - }) - } + + // get_device_attr should be marked as unsafe, and will be in future. + // SAFETY: gicv3_its_attr.addr is safe to write to. + its_device + .get_device_attr(&mut gicv3_its_attr) + .map_err(|e| { + Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into())) + })?; + + Ok(val) } /// Function that saves/restores ITS tables into guest RAM. @@ -324,60 +328,43 @@ impl Vgic for KvmGicV3Its { let icc_state = get_icc_regs(&self.device, &gicr_typers)?; - let its_baser_state: [u64; 8] = [0; 8]; + let mut its_baser_state: [u64; 8] = [0; 8]; for i in 0..8 { - gicv3_its_attr_access( + its_baser_state[i as usize] = gicv3_its_attr_get( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_BASER + i * 8, - &its_baser_state[i as usize], - false, )?; } - let its_ctlr_state: u64 = 0; - gicv3_its_attr_access( + let its_ctlr_state = gicv3_its_attr_get( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CTLR, - &its_ctlr_state, - false, )?; - let its_cbaser_state: u64 = 0; - gicv3_its_attr_access( + let its_cbaser_state = gicv3_its_attr_get( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CBASER, - &its_cbaser_state, - false, )?; - let its_creadr_state: u64 = 0; - gicv3_its_attr_access( + let its_creadr_state = gicv3_its_attr_get( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CREADR, - &its_creadr_state, - false, )?; - let its_cwriter_state: u64 = 0; - gicv3_its_attr_access( + let its_cwriter_state = gicv3_its_attr_get( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CWRITER, - &its_cwriter_state, - false, )?; - let its_iidr_state: u64 = 0; - gicv3_its_attr_access( + let its_iidr_state = gicv3_its_attr_get( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_IIDR, - &its_iidr_state, - false, )?; Ok(Gicv3ItsState { @@ -407,57 +394,51 @@ impl Vgic for KvmGicV3Its { set_icc_regs(&self.device, &gicr_typers, &state.icc)?; //Restore GICv3ITS registers - gicv3_its_attr_access( + gicv3_its_attr_set( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_IIDR, - &state.its_iidr, - true, + state.its_iidr, )?; - gicv3_its_attr_access( + gicv3_its_attr_set( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CBASER, - &state.its_cbaser, - true, + state.its_cbaser, )?; - gicv3_its_attr_access( + gicv3_its_attr_set( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CREADR, - &state.its_creadr, - true, + state.its_creadr, )?; - gicv3_its_attr_access( + gicv3_its_attr_set( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CWRITER, - &state.its_cwriter, - true, + state.its_cwriter, )?; for i in 0..8 { - gicv3_its_attr_access( + gicv3_its_attr_set( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_BASER + i * 8, - &state.its_baser[i as usize], - true, + state.its_baser[i as usize], )?; } // Restore ITS tables gicv3_its_tables_access(self.its_device.as_ref().unwrap(), false)?; - gicv3_its_attr_access( + gicv3_its_attr_set( self.its_device.as_ref().unwrap(), kvm_bindings::KVM_DEV_ARM_VGIC_GRP_ITS_REGS, GITS_CTLR, - &state.its_ctlr, - true, + state.its_ctlr, ) } diff --git a/hypervisor/src/kvm/aarch64/gic/redist_regs.rs b/hypervisor/src/kvm/aarch64/gic/redist_regs.rs index 4ff17ac5d..213a57099 100644 --- a/hypervisor/src/kvm/aarch64/gic/redist_regs.rs +++ b/hypervisor/src/kvm/aarch64/gic/redist_regs.rs @@ -96,23 +96,38 @@ static VGIC_SGI_REGS: &[RdistReg] = &[ VGIC_RDIST_REG!(GICR_IPRIORITYR0, 32), ]; -fn redist_attr_access(gic: &DeviceFd, offset: u32, typer: u64, val: &u32, set: bool) -> Result<()> { +fn redist_attr_set(gic: &DeviceFd, offset: u32, typer: u64, val: u32) -> Result<()> { + let gic_redist_attr = kvm_device_attr { + group: KVM_DEV_ARM_VGIC_GRP_REDIST_REGS, + attr: (typer & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) | (offset as u64), // this needs the mpidr + addr: &val as *const u32 as u64, + flags: 0, + }; + + gic.set_device_attr(&gic_redist_attr).map_err(|e| { + Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into())) + })?; + + Ok(()) +} + +fn redist_attr_get(gic: &DeviceFd, offset: u32, typer: u64) -> Result { + let mut val = 0; + let mut gic_redist_attr = kvm_device_attr { group: KVM_DEV_ARM_VGIC_GRP_REDIST_REGS, attr: (typer & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) | (offset as u64), // this needs the mpidr - addr: val as *const u32 as u64, + addr: &mut val as *mut u32 as u64, flags: 0, }; - if set { - gic.set_device_attr(&gic_redist_attr).map_err(|e| { - Error::SetDeviceAttribute(HypervisorDeviceError::SetDeviceAttribute(e.into())) - })?; - } else { - gic.get_device_attr(&mut gic_redist_attr).map_err(|e| { - Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into())) - })?; - } - Ok(()) + + // get_device_attr should be marked as unsafe, and will be in future. + // SAFETY: gic_redist_attr.addr is safe to write to. + gic.get_device_attr(&mut gic_redist_attr).map_err(|e| { + Error::GetDeviceAttribute(HypervisorDeviceError::GetDeviceAttribute(e.into())) + })?; + + Ok(val) } fn access_redists_aux( @@ -129,14 +144,11 @@ fn access_redists_aux( let end = base + rdreg.length as u32; while base < end { - let mut val = 0; if set { - val = state[*idx]; - redist_attr_access(gic, base, *i, &val, true)?; + redist_attr_set(gic, base, *i, state[*idx])?; *idx += 1; } else { - redist_attr_access(gic, base, *i, &val, false)?; - state.push(val); + state.push(redist_attr_get(gic, base, *i)?); } base += REG_SIZE as u32; }