Skip to content

hypervisor: kvm: aarch64: fix get_device_attr() UB#6647

Merged
liuw merged 1 commit intocloud-hypervisor:mainfrom
alyssais:hypervisor-ub
Aug 13, 2024
Merged

hypervisor: kvm: aarch64: fix get_device_attr() UB#6647
liuw merged 1 commit intocloud-hypervisor:mainfrom
alyssais:hypervisor-ub

Conversation

@alyssais
Copy link
Member

DeviceFd::get_device_attr should be marked as unsafe, because it allows writing to an arbitrary address. I have opened a kvm-ioctls PR 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 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.

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>
Copy link
Member

@likebreath likebreath left a comment

Choose a reason for hiding this comment

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

Thank you for reporting and fixing the issue.

@rbradford rbradford added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@alyssais
Copy link
Member Author

Presumably test_vfio is just flaky, given it failed on x86_64 and this code only touched aarch64-specific code?

@liuw liuw added this pull request to the merge queue Aug 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 13, 2024
@alyssais
Copy link
Member Author

Last time it failed on musl, and worked on GNU, and this time it's the opposite…

@liuw liuw added this pull request to the merge queue Aug 13, 2024
Merged via the queue into cloud-hypervisor:main with commit 02f146f Aug 13, 2024
@alyssais alyssais deleted the hypervisor-ub branch August 14, 2024 07:49
@rbradford rbradford added bug-fix Bug fix to include in release notes AArch64 Affects AArch64 only labels Aug 15, 2024
@alyssais alyssais mentioned this pull request Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AArch64 Affects AArch64 only bug-fix Bug fix to include in release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants