Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot #4987

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ and this project adheres to

### Added

- [#4987](https://github.com/firecracker-microvm/firecracker/pull/4987): Reset
physical counter register (`CNTPCT_EL0`) on VM startup. This avoids VM reading
the host physical counter value. This is only possible on 6.4 and newer
kernels. For older kernels a warning will be printed to the logs.

### Changed

- [#4913](https://github.com/firecracker-microvm/firecracker/pull/4913): Removed
Expand Down
12 changes: 6 additions & 6 deletions docs/prod-host-setup.md
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,13 @@ For vendor-specific recommendations, please consult the resources below:
- ARM:
[Speculative Processor Vulnerability](https://developer.arm.com/support/arm-security-updates/speculative-processor-vulnerability)

##### [ARM only] Physical counter directly passed through to the guest
##### [ARM only] VM Physical counter reset on VM boot

On ARM, the physical counter (i.e `CNTPCT`) it is returning the
[actual EL1 physical counter value of the host][1]. From the discussions before
merging this change [upstream][2], this seems like a conscious design decision
of the ARM code contributors, giving precedence to performance over the ability
to trap and control this in the hypervisor.
On ARM, Firecracker tries to reset the `CNTPCT` physical counter on VM boot.
This is done in order to prevent VM from reading host physical counter value.
Because this is only possible in kernels containing [this](https://lore.kernel.org/all/[email protected]/)
patch series (6.4 and newer) Firecracker does not fail if it cannot reset the counter
and instead prints a warning message.

##### Verification

Expand Down
6 changes: 6 additions & 0 deletions src/vmm/src/arch/aarch64/regs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ arm64_sys_reg!(SYS_CNTV_CVAL_EL0, 3, 3, 14, 3, 2);
// https://elixir.bootlin.com/linux/v6.8/source/arch/arm64/include/asm/sysreg.h#L459
arm64_sys_reg!(SYS_CNTPCT_EL0, 3, 3, 14, 0, 1);

// Physical Timer EL0 count Register
ShadowCurse marked this conversation as resolved.
Show resolved Hide resolved
// The id of this register is same as SYS_CNTPCT_EL0, but KVM defines it
// separately, so we do as well.
// https://elixir.bootlin.com/linux/v6.12.6/source/arch/arm64/include/uapi/asm/kvm.h#L259
arm64_sys_reg!(KVM_REG_ARM_PTIMER_CNT, 3, 3, 14, 0, 1);

// Translation Table Base Register
// https://developer.arm.com/documentation/ddi0595/2021-03/AArch64-Registers/TTBR1-EL1--Translation-Table-Base-Register-1--EL1-
arm64_sys_reg!(TTBR1_EL1, 3, 0, 2, 0, 1);
Expand Down
28 changes: 28 additions & 0 deletions src/vmm/src/arch/aarch64/vcpu.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use kvm_bindings::*;
use kvm_ioctls::VcpuFd;
use log::warn;

use super::get_fdt_addr;
use super::regs::*;
Expand Down Expand Up @@ -106,6 +107,24 @@
vcpufd
.set_one_reg(id, &get_fdt_addr(mem).to_le_bytes())
.map_err(|err| VcpuError::SetOneReg(id, err))?;

// Reset the physical counter for the guest. This way we avoid guest reading
// host physical counter.
// Resetting KVM_REG_ARM_PTIMER_CNT for single vcpu is enough because there is only
// one timer struct with offsets per VM.
// Because the access to KVM_REG_ARM_PTIMER_CNT is only present starting 6.4 kernel,
ShadowCurse marked this conversation as resolved.
Show resolved Hide resolved
// we don't fail if ioctl returns an error.
// Path series which introduced the needed changes:
// https://lore.kernel.org/all/[email protected]/
// Note: the value observed by the guest will still be above 0, because there is a delta
// time between this resetting and first call to KVM_RUN.
let zero: u64 = 0;
if vcpufd
.set_one_reg(KVM_REG_ARM_PTIMER_CNT, &zero.to_le_bytes())
.is_err()
{
warn!("Unable to reset VM physical counter. VM will use host value instead.");
}

Check warning on line 127 in src/vmm/src/arch/aarch64/vcpu.rs

View check run for this annotation

Codecov / codecov/patch

src/vmm/src/arch/aarch64/vcpu.rs#L127

Added line #L127 was not covered by tests
}
Ok(())
}
Expand Down Expand Up @@ -238,6 +257,15 @@
vcpu.vcpu_init(&kvi).unwrap();

setup_boot_regs(&vcpu, 0, 0x0, &mem).unwrap();

// Check that the register is reset on compatible kernels.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add an integration test in tests/integration_tests/security?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integration test will be quite difficult as we will need kernel access to read the physical counter.

// Because there is a delta in time between we reset the register and time we
// read it, we cannot compare with 0. Choose some meaningfully small value instead.
let mut reg_bytes = [0_u8; 8];
if vcpu.get_one_reg(SYS_CNTPCT_EL0, &mut reg_bytes).is_ok() {
let counter_value = u64::from_le_bytes(reg_bytes);
assert!(counter_value < 10_000);
}
}

#[test]
Expand Down
Loading