containers / libkrun

A dynamic library providing Virtualization-based process isolation capabilities
Apache License 2.0
901 stars 74 forks source link

SEV "sev" library usage and introduced SEV-SNP support #82

Closed tylerfanelli closed 2 years ago

tylerfanelli commented 2 years ago

This PR adds a few new features to the TEE capabilities of libkrun:

One thing missing in this PR is that although SEV-SNP VMs can be created, there is no interface for attesting a SEV-SNP VM. Attestation support is currently a WIP.

tylerfanelli commented 2 years ago

CC @slp

slp commented 2 years ago

Nice work here! A couple of comments:

tylerfanelli commented 2 years ago

@slp

Nice work here! A couple of comments:

  • Please update arch/src/x86_64/ references from amd-sev to tee, as we need this behavior on SNP and TDX too.

Sure, will re-push with these changes.

  • We need to measure more regions for SNP, but I'm inclined to merge this as is, and the update that on a later PR, if you're fine with it.

If you're comfortable doing that, then I'm fine with it. What other regions (besides our previously discussed CPUID page) are you referring to?

tylerfanelli commented 2 years ago

@slp All amd-sev references in arch/src/x86_64 references have been changed to tee

slp commented 2 years ago

Now that we have a unified kernel+qboot-krunfw bundle that works for both SEV and SNP, I think we should update this PR to use a single feature for enabling both amd-sev and amd-snp. Probably the most reasonable way is keeping amd-sev and having it enable both SEV and SNP code paths. Then, the decision to use one or the other can be taken using the field tee from TeeConfig.

That said, I would keep the tee build conditionals as they are, as they'll be useful once we add TDX support next.

tylerfanelli commented 2 years ago

So, essentially what you are saying is to remove the amd-snp flag altogether, and encase all SNP code/data in the amd-sev config instead. So although both code paths will be included, the paths taken for encryption would be handled by the TeeConfig.tee field? Am I understanding correctly?

slp commented 2 years ago

So, essentially what you are saying is to remove the amd-snp flag altogether, and encase all SNP code/data in the amd-sev config instead. So although both code paths will be included, the paths taken for encryption would be handled by the TeeConfig.tee field? Am I understanding correctly?

Exactly. Keeping what in the current state of this PR is tee as tee, so we can reuse that work for introducing TDX later on.

slp commented 2 years ago

@tylerfanelli Should we merge this one as-is and improve on top of it, or do we want to improve it beforehand?

tylerfanelli commented 2 years ago

@slp I've added some commits and tested them on virtlab. They work nicely with the updated libkrunfw.

tylerfanelli commented 2 years ago

@slp My latest commit fixes the cargo test issue. All checks are now passing.

slp commented 2 years ago

@tylerfanelli I've made some changes to the PR, please take a look.

tylerfanelli commented 2 years ago

LGTM. I'm fine with these changes.