External/cloud-hypervisor
1
0
Fork 0
mirror of https://github.com/cloud-hypervisor/cloud-hypervisor.git synced 2024-12-22 13:45:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

hypervisor: kvm: aarch64: fix get_device_attr() UB

Browse Source 
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]: d2d24e395a

Signed-off-by: Alyssa Ross <hi@alyssa.is>
This commit is contained in:
Alyssa Ross 2024-08-12 17:06:00 +02:00 committed by Liu Wei
parent 36cdd67b9c
commit 02f146fef8
4 changed files with 144 additions and 128 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
hypervisor/src/kvm/aarch64/gic/dist_regs.rs
View File

@ -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<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: 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<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;
}
}

57
hypervisor/src/kvm/aarch64/gic/icc_regs.rs
View File

@ -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<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: 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<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,8 +144,7 @@ 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
@ -139,12 +152,10 @@ pub fn get_icc_regs(gic: &DeviceFd, gicr_typer: &[u64]) -> Result<Vec<u32>> {
|| *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;
}
}

111
hypervisor/src/kvm/aarch64/gic/mod.rs
View File

@ -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<u64> {
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,
)
}

46
hypervisor/src/kvm/aarch64/gic/redist_regs.rs
View File

@ -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<u32> {
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;
}