Skip to content

Add support for KVM_GET_XSAVE2 #310

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

Merged
merged 2 commits into from
Mar 6, 2025
Merged

Conversation

zulinx86
Copy link
Contributor

@zulinx86 zulinx86 commented Mar 6, 2025

Summary of the PR

Since Linux 5.17, the kvm_xsave struct has a flexiblre array member
(FAM) at the end, which can be retrieved using the KVM_GET_XSAVE2
ioctl [1]. What makes this FAM special is that the length is not stored
in itself, but has to be retrieved via
KVM_CHECK_CAPABILITY(KVM_CAP_XSAVE2) which returns the total size of
the kvm_xsave struct (e.g. the traditional 4096 byte region + extra
region with the size of the FAM). To support it in rust-vmm, Xsave has
been introduced as its FamStructWrapper.

The size required to hold the whole kvm_xsave structure can vary
depending on features that have been dynamically enabled by
arch_prctl() [2]. Any features must not be enabled after the size has
been confirmed; otherwise, KVM_GET_XSAVE2 writes beyond the allocated
area for Xsave, potentially causing undefined behavior. It is unable
to put KVM_CHECK_CAPABILITY call for the size check and
KVM_GET_XSAVE2 call together into get_xsave2(), because there is a
chance of race condition where another thread enables additional
features between them. That's why get_xsave2() is marked unsafe.

Although KVM_SET_XSAVE was extended and KVM_SET_XSAVE2 was not added
to support the kvm_xsave with FAM, set_xsave2() is also implemented
to enable users to pass Xsave to it for convenience. That is also
marked unsafe for the same reason.

In addition to that, after Linux 5.17, the existing set_xsave2() may copy data
beyond the traditional 4096 bytes if XSTATE features are dynamically enabled.
So it is also marked unsafe.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

Since Linux 5.17, the `kvm_xsave` struct has a flexiblre array member
(FAM) at the end, which can be retrieved using the `KVM_GET_XSAVE2`
ioctl [1]. What makes this FAM special is that the length is not stored
in itself, but has to be retrieved via
`KVM_CHECK_CAPABILITY(KVM_CAP_XSAVE2)` which returns the total size of
the `kvm_xsave` struct (e.g. the traditional 4096 byte region + extra
region with the size of the FAM). To support it in rust-vmm, `Xsave` has
been introduced as its `FamStructWrapper`.

The size required to hold the whole `kvm_xsave` structure can vary
depending on features that have been dynamically enabled by
`arch_prctl()` [2]. Any features must not be enabled after the size has
been confirmed; otherwise, `KVM_GET_XSAVE2` writes beyond the allocated
area for `Xsave`, potentially causing undefined behavior. It is unable
to put `KVM_CHECK_CAPABILITY` call for the size check and
`KVM_GET_XSAVE2` call together into `get_xsave2()`, because there is a
chance of race condition where another thread enables additional
features between them. That's why `get_xsave2()` is marked `unsafe`.

Although `KVM_SET_XSAVE` was extended and `KVM_SET_XSAVE2` was not added
to support the `kvm_xsave` with FAM, `set_xsave2()` is also implemented
to enable users to pass `Xsave` to it for convenience. That is also
marked `unsafe` for the same reason.

[1]: https://www.kernel.org/doc/html/latest/virt/kvm/api.html#kvm-get-xsave2
[2]: https://docs.kernel.org/arch/x86/xstate.html

Co-authored-by: Patrick Roy <roypat@maazon.co.uk>
Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
roypat
roypat previously approved these changes Mar 6, 2025
The C `kvm_xsave` strut was extended to have a flexible array member
(FAM) at the end in Linux 5.17. The size can vary depending on features
that have been dynamically enabled via `arch_prctl()` and the required
size can be retrieved via `KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2)`. That
means `KVM_SET_XSAVE` may copy data beyond the size of the traditional C
`kvm_xsave` struct (i.e. 4096 bytes) now, possibly causing undefined
behavior.

It is safe if used on Linux prior to 5.17, if no XSTATE features are
enabled dynamically or if the required size is still within the
traditional 4096 bytes even with dynamically enabled features. However,
if any features are enabled dynamically, it is recommended to use
`set_xsave2()` instead.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
@roypat roypat requested a review from rbradford March 6, 2025 17:37
@ShadowCurse ShadowCurse merged commit bae8bc7 into rust-vmm:main Mar 6, 2025
39 checks passed
@zulinx86 zulinx86 deleted the KVM_GET_XSAVE2 branch March 7, 2025 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants