Enable `KVM_MEM_ENCRYPT_OP` ioctl for AMD SEV #111

kailun-qin · 2020-08-04T02:51:48Z

The KVM_MEM_ENCRYPT_OP ioctl is used to access AMD SEV (Secure
Encrypted Virtualization) feature. When enabled, memory contents of a VM
will be transparently encrypted with a key unique to that VM.

This patch added implementation, documentation and related tests for the
following SEV commands:

KVM_SEV_INIT
KVM_SEV_LAUNCH_START
KVM_SEV_LAUNCH_UPDATE_DATA
KVM_SEV_LAUNCH_MEASURE
KVM_SEV_LAUNCH_FINISH
KVM_SEV_LAUNCH_SECRET

Note: this work is based on enarx/ketuvim. See [1]_ for details.

[1] rust-vmm/community#62

Signed-off-by: Kailun Qin [email protected]

The `KVM_MEM_ENCRYPT_OP` ioctl is used to access AMD SEV (Secure Encrypted Virtualization) feature. When enabled, memory contents of a VM will be transparently encrypted with a key unique to that VM. This patch added implementation, documentation and related tests for the following SEV commands: - KVM_SEV_INIT - KVM_SEV_LAUNCH_START - KVM_SEV_LAUNCH_UPDATE_DATA - KVM_SEV_LAUNCH_MEASURE - KVM_SEV_LAUNCH_FINISH - KVM_SEV_LAUNCH_SECRET Note: this work is based on enarx/ketuvim. See [1]_ for details. [1] rust-vmm/community#62 Signed-off-by: Kailun Qin <[email protected]>

jiangliu · 2020-08-04T06:33:16Z

This introduces too much dependency crates:(

kailun-qin · 2020-08-04T09:59:50Z

This introduces too much dependency crates:(

Hi @jiangliu ,

Thanks for the comments.

Most of the dependency crates introduced are dev-only and the rest for runtime are feature-controlled.

For the test-only dependencies, a large part of them aim to facilitate users with the construction of SEV cert chain (from remote). We can get rid of them if we assume that users have already had their own available locally. This depends on how we trade-off and I'd like to see more feedbacks.

Actually, the only one runtime dependency needed is the sev crate from enarx project and let's see how the enarx community suggests.

alxiord

Apart from some code quirks (left comments), my main concern is how to consume the sev crate dependency (currently a crate in enarx) 🤔

alxiord · 2020-09-03T13:13:19Z

Cargo.toml

@@ -12,6 +12,19 @@ license = "Apache-2.0 OR MIT"
 libc = ">=0.2.39"
 kvm-bindings = { version = ">=0.2.0", features = ["fam-wrappers"] }
 vmm-sys-util = ">=0.2.1"
+bitflags = { version = ">=1.2.1", optional = true }
+sev = { path = "./sev", features = ["openssl"], optional = true }


There's no sev crate, does this refer to the sev mod?

~~Or did you have the sev crate locally in your workspace? The PR doesn't build without it.~~
LE: I think this is the sev crate from the enarx repo; but, as enarx doesn't have a Cargo.toml (they seem to use a different build system, cargo make?), Cargo can't get the crate dependency right with

sev = { git = "https://github.com/enarx/enarx.git", features = ["openssl"], optional = true }

This would be a blocker in publishing a new version of kvm-ioctls with sev support: the sev crate needs to be published first.
In the meantime, for playing around, maybe you could get away with a multiple-dependency like explained here. Cargo would look for the sev crate on crates.io and, until it finds it there, pick it up from the local repo (to get the CI to pass, you'd have to yank it into your fork and add it to the PR).

What's more, maybe you should also restrict the sev dependencies to target=x86_64 as they won't build on ARM.

~~Or did you have the sev crate locally in your workspace? The PR doesn't build without it.~~
LE: I think this is the sev crate from the enarx repo; but, as enarx doesn't have a Cargo.toml (they seem to use a different build system, cargo make?), Cargo can't get the crate dependency right with

sev = { git = "https://github.com/enarx/enarx.git", features = ["openssl"], optional = true }

This would be a blocker in publishing a new version of kvm-ioctls with sev support: the sev crate needs to be published first.
In the meantime, for playing around, maybe you could get away with a multiple-dependency like explained here. Cargo would look for the sev crate on crates.io and, until it finds it there, pick it up from the local repo (to get the CI to pass, you'd have to yank it into your fork and add it to the PR).

Yes, I do have a modified sev crate in my workspace, which also implements the traits to resolve compilation issues you mentioned below. (that is why I marked this PR as a draft version ;))
As enarx team has cited in another thread. Let's wait a moment for their coming release of sev crate and APIs. I'll have an update based on that.

What's more, maybe you should also restrict the sev dependencies to target=x86_64 as they won't build on ARM.

Yes agree, will update.

alxiord · 2020-09-03T13:33:49Z

Cargo.toml

+lazy_static = "1.4.0"
+
+[features]
+default = ["amd-sev"]


I don't think this should be a default feature.

Right, will remove the default.

alxiord · 2020-09-03T13:35:46Z

src/ioctls/sev.rs

+/// Learn from ``SevState`` introduced in QEMU, adding few changes.
+/// An enumeration of SEV state information used during @query-sev.
+///
+/// @Uninit: The guest is uninitialized.


nit: these should be converted to doc comments.

alxiord · 2020-09-03T13:37:32Z

src/ioctls/sev.rs

+    pub fn start(self, start: Start) -> Result<SevLaunch<LaunchUpdate>> {
+        let start = kvm_sev_launch_start {
+            handle: 0,
+            policy: start.policy.into(),


This doesn't compile on my machine because there's no Into<u32> for Policy.

alxiord · 2020-09-03T13:38:17Z

src/ioctls/sev.rs

+    /// The guest owner may wait to provide the guest with confidential information until
+    /// it can verify the measurement thru comparison.
+    pub fn measure(self) -> Result<SevLaunch<LaunchSecret>> {
+        let mut mr = Measurement::default();


This doesn't compile on my machine because there's no impl Default for Measurement.

alxiord · 2020-09-03T13:44:20Z

src/ioctls/sev.rs

+
+impl SevLaunch<LaunchSecret> {
+    /// Get the measurement kept by the SEV launch context (in a `LaunchSecret` state)
+    pub fn get_measure(&self) -> Measurement {


nit: getters usually lack the get_ prefix in Rust

alxiord · 2020-09-03T13:45:53Z

src/ioctls/sev.rs

+        self.sev_ioctl(SevCmd::LaunchSecret, secret)?;
+        Ok(())


Suggested change

self.sev_ioctl(SevCmd::LaunchSecret, secret)?;

Ok(())

self.sev_ioctl(SevCmd::LaunchSecret, secret)

alxiord · 2020-09-03T13:52:21Z

src/ioctls/sev.rs

+///
+/// @ReceiveUpdate: The guest is currently being migrated from another machine.
+#[derive(PartialEq, Debug)]
+pub struct Uninit;


This is meant to be an enumeration, so why not express it as an Enum? Also, modelling the state transitions with the From trait is a bit weird to me, and not very idiomatic IMO - From conceptually implies equivalence between the 2 operands, while a state machine implies not just non-equivalence, but order between the states. I believe an Enum here and explicit transition functions between the states would make the code clearer.

This is meant to be an enumeration, so why not express it as an Enum? Also, modelling the state transitions with the From trait is a bit weird to me, and not very idiomatic IMO - From conceptually implies equivalence between the 2 operands, while a state machine implies not just non-equivalence, but order between the states. I believe an Enum here and explicit transition functions between the states would make the code clearer.

Thanks for the suggestions. Seems reasonable to me. I'll make an update in the next version.

alxiord · 2020-09-03T13:53:01Z

src/ioctls/vm.rs

+    pub fn create_sev_launch(self) -> Result<SevLaunch<Init>> {
+        let launch = SevLaunch::new_with_vm(self)?;
+
+        Ok(launch.init()?)


Suggested change

Ok(launch.init()?)

launch.init()

alxiord · 2020-09-03T13:53:28Z

src/kvm_ioctls.rs

+/* Memory Encryption Commands */
+/* KVM_MEMORY_ENCRYPT_OP */
+#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+ioctl_iowr_nr!(KVM_MEMORY_ENCRYPT_OP, KVMIO, 0xba, c_ulong);


Should this be gated to feature = "amd-sev" too?

Thanks, this should be gated to feature = "amd-sev".

lauralt · 2020-10-07T06:29:14Z

Hi, @kailun-qin! Are you still interested in merging this PR?

kailun-qin · 2020-10-08T03:55:37Z

Hi, @kailun-qin! Are you still interested in merging this PR?

Hi @lauralt ,
Thanks for following this up!
Sure, I'll continue working on this PR once enarx team stabilizes the dependent sev and publishes a release in the next few days. Please kindly see the previous discussions in another thread.

andreeaflorescu · 2021-11-17T16:14:37Z

This support was merged in #178. Closing this.

kailun-qin requested review from acatangiu, alxiord, andreeaflorescu and sameo as code owners August 4, 2020 02:51

kailun-qin marked this pull request as draft August 4, 2020 03:03

kailun-qin mentioned this pull request Aug 4, 2020

Collaboration / Merge with Ketuvim rust-vmm/community#62

Open

alxiord reviewed Sep 3, 2020

View reviewed changes

rvolosatovs mentioned this pull request Oct 13, 2021

Add support for KVM_MEMORY_ENCRYPT_{OP,REG_REGION,UNREG_REGION} #178

Merged

andreeaflorescu closed this Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `KVM_MEM_ENCRYPT_OP` ioctl for AMD SEV #111

Enable `KVM_MEM_ENCRYPT_OP` ioctl for AMD SEV #111

kailun-qin commented Aug 4, 2020

jiangliu commented Aug 4, 2020

kailun-qin commented Aug 4, 2020

alxiord left a comment •

edited

Loading

alxiord Sep 3, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

kailun-qin Sep 4, 2020

kailun-qin Sep 4, 2020

alxiord Sep 3, 2020

kailun-qin Sep 4, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

kailun-qin Sep 4, 2020

alxiord Sep 3, 2020

alxiord Sep 3, 2020

kailun-qin Sep 4, 2020

lauralt commented Oct 7, 2020

kailun-qin commented Oct 8, 2020

andreeaflorescu commented Nov 17, 2021

	self.sev_ioctl(SevCmd::LaunchSecret, secret)?;
	Ok(())
	self.sev_ioctl(SevCmd::LaunchSecret, secret)

Enable KVM_MEM_ENCRYPT_OP ioctl for AMD SEV #111

Enable KVM_MEM_ENCRYPT_OP ioctl for AMD SEV #111

Conversation

kailun-qin commented Aug 4, 2020

jiangliu commented Aug 4, 2020

kailun-qin commented Aug 4, 2020

alxiord left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauralt commented Oct 7, 2020

kailun-qin commented Oct 8, 2020

andreeaflorescu commented Nov 17, 2021

Enable `KVM_MEM_ENCRYPT_OP` ioctl for AMD SEV #111

Enable `KVM_MEM_ENCRYPT_OP` ioctl for AMD SEV #111

alxiord left a comment •

edited

Loading