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

move encrypt_op{_sev} into system ioctls #259

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakecorrenti
Copy link

Summary of the PR

According to the KVM API, KVM_MEMORY_ENCRYPT_OP is labeled as a system ioctl. However, it is currently treated as a VM ioctl. There are no functional changes to the code, only the location and function signature.

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.

Based on the KVM API, KVM_MEMORY_ENCRYPT_OP is a system ioctl. However,
this was not reflected in the crate.

Signed-off-by: Jake Correnti <[email protected]>
@jakecorrenti
Copy link
Author

Just for reference, this is the documentation that states the KVM API now uses MEMORY_ENCRYPT_OP as a system ioctl: https://www.kernel.org/doc/Documentation/virtual/kvm/api.txt

Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Sorry for the late review, my GitHub search was not showing this one for some reason :(

@@ -7,6 +7,8 @@
- [[#255](https://github.com/rust-vmm/kvm-ioctls/issues/255)]: Fixed a
soundness issue when accessing the `kvm_run` struct. `VcpuFd::run()` and
`VcpuFd::set_kvm_immediate_exit()` now take `&mut self` as a consequence.
- Changed `encrypt_op` and `encrypt_op_sev` to system ioctls to be in line
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add the PR number here?

Suggested change
- Changed `encrypt_op` and `encrypt_op_sev` to system ioctls to be in line
- [[#259](https://github.com/rust-vmm/kvm-ioctls/pull/259)] Changed `encrypt_op` and `encrypt_op_sev` to system ioctls to be in line

Comment on lines +724 to +725
pub unsafe fn encrypt_op<T>(&self, fd: &impl AsRawFd, op: *mut T) -> Result<()> {
let ret = ioctl_with_mut_ptr(fd, KVM_MEMORY_ENCRYPT_OP(), op);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not familiar with AMD-SEV, but if this is a system ioctl, shouldn't it be issued on the KVM fd? e.g.

Suggested change
pub unsafe fn encrypt_op<T>(&self, fd: &impl AsRawFd, op: *mut T) -> Result<()> {
let ret = ioctl_with_mut_ptr(fd, KVM_MEMORY_ENCRYPT_OP(), op);
pub unsafe fn encrypt_op<T>(&self, op: *mut T) -> Result<()> {
let ret = ioctl_with_mut_ptr(&self.kvm, KVM_MEMORY_ENCRYPT_OP(), op);

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I believe you're right. It was brought to my attention that I was referencing stale documentation. However, the changes to the ioctl should be so that it is both a vm ioctl and a vcpu ioctl, so that requires an update on my end.

@rbradford
Copy link
Contributor

Just for reference, this is the documentation that states the KVM API now uses MEMORY_ENCRYPT_OP as a system ioctl: https://www.kernel.org/doc/Documentation/virtual/kvm/api.txt

Not sure how old that documentation is - but v6.9 - the last release version shows this is a vm type ioctl - https://elixir.bootlin.com/linux/v6.9/source/Documentation/virt/kvm/api.rst#L4732

The wrong label was fixed in torvalds/linux@46ca9ee

@rbradford
Copy link
Contributor

I identified the issue as a case of stale content on kernel.org when the directory was renamed by

commit 2f5947dfcaecb99f2dd559156eecbeb7b95e4c02
Author: Christoph Hellwig <[email protected]>
Date:   Wed Jul 24 09:24:49 2019 +0200

    Documentation: move Documentation/virtual to Documentation/virt
    
    Renaming docs seems to be en vogue at the moment, so fix on of the
    grossly misnamed directories.  We usually never use "virtual" as
    a shortcut for virtualization in the kernel, but always virt,
    as seen in the virt/ top-level directory.  Fix up the documentation
    to match that.
    
    Fixes: ed16648eb5b8 ("Move kvm, uml, and lguest subdirectories under a common "virtual" directory, I.E:")
    Signed-off-by: Christoph Hellwig <[email protected]>
    Signed-off-by: Paolo Bonzini <[email protected]>

Compare - https://www.kernel.org/doc/Documentation/virt/kvm/api.txt (current) with https://www.kernel.org/doc/Documentation/virtual/kvm/api.txt (stale)

I have contacted the kernel.org team to highlight the issue.

@jakecorrenti
Copy link
Author

jakecorrenti commented May 22, 2024

I identified the issue as a case of stale content on kernel.org when the directory was renamed by

commit 2f5947dfcaecb99f2dd559156eecbeb7b95e4c02
Author: Christoph Hellwig [email protected]
Date: Wed Jul 24 09:24:49 2019 +0200

Documentation: move Documentation/virtual to Documentation/virt

Renaming docs seems to be en vogue at the moment, so fix on of the
grossly misnamed directories.  We usually never use "virtual" as
a shortcut for virtualization in the kernel, but always virt,
as seen in the virt/ top-level directory.  Fix up the documentation
to match that.

Fixes: ed16648eb5b8 ("Move kvm, uml, and lguest subdirectories under a common "virtual" directory, I.E:")
Signed-off-by: Christoph Hellwig <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>

Compare - https://www.kernel.org/doc/Documentation/virt/kvm/api.txt (current) with https://www.kernel.org/doc/Documentation/virtual/kvm/api.txt (stale)

I have contacted the kernel.org team to highlight the issue.

Thank you for doing that.

However, I still see this change being needed (with some adjustments). Intel TDX (and I think Arm CCA as well) is repurposing this ioctl and it is used on both Vm fd's and Vcpu fd's. Intel's documentation about it can be located here.

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