search

coconut-svsm / svsm

COCONUT-SVSM
MIT License
123 stars 43 forks source link

SVSM: require SNP restricted injection #259

Open msft-jlange opened 9 months ago

msft-jlange commented 9 months ago

To maintain security of the SVSM, require that Restricted Injection is present in the SEV features in order to boot.

roy-hopkins commented 9 months ago

I tested this on the latest IGVM QEMU and it results in a panic on SVSM startup due to the SEV features not being propagated from the IGVM file via QEMU. I'll look at fixing this in the IGVM QEMU.

Zildj1an commented 9 months ago

Requiring restricted injection is a good idea and it doesn't affect the guest itself! :+1:

roy-hopkins commented 9 months ago

By default, QEMU does not enable restricted injection. It can be enabled via a parameter on the command line. At the moment this is true for both the IGVM and non-IGVM versions of QEMU.

Maybe you could add a commit to this PR that updates the documentation:

diff --git a/Documentation/INSTALL.md b/Documentation/INSTALL.md
index 477b4d1..35931a0 100644
--- a/Documentation/INSTALL.md
+++ b/Documentation/INSTALL.md
@@ -225,7 +225,7 @@ guest:
   -cpu EPYC-v4 \
   -machine q35,confidential-guest-support=sev0,memory-backend=ram1,kvm-type=protected \
   -object memory-backend-memfd-private,id=ram1,size=8G,share=true \
-  -object sev-snp-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,svsm=on \
+  -object sev-snp-guest,id=sev0,cbitpos=51,reduced-phys-bits=1,svsm=on,init-flags=1 \

This selects the EPYC-v4 CPU type which will pass the CPUID validation @@ -233,7 +233,9 @@ done by the AMD security processor. It also allocates memory from the memory-backend-memfd-private backend, which is a requirement to run SEV-SNP guests. An sev-snp-guest object needs to be defined to enable SEV-SNP protection for the guest. The 'svsm=on' parameter makes -QEMU reserve a small amount of guest memory for the SVSM. +QEMU reserve a small amount of guest memory for the SVSM. The init-flags=1 +parameter enables the SEV-SNP restricted injection feature which is required +by COCONUT-SVSM.

Further, the OVMF binaries and the SVSM binary need to be passed to QEMU. This happens via pflash drives: @@ -255,7 +257,7 @@ $ sudo $HOME/bin/qemu-svsm/bin/qemu-system-x86_64 \ -cpu EPYC-v4 \ -machine q35,confidential-guest-support=sev0,memory-backend=ram1,kvm-type=protected \ -object memory-backend-memfd-private,id=ram1,size=8G,share=true \

msft-jlange commented 9 months ago

By default, QEMU does not enable restricted injection. It can be enabled via a parameter on the command line. At the moment this is true for both the IGVM and non-IGVM versions of QEMU.

Maybe you could add a commit to this PR that updates the documentation:

I think a better strategy would be to modify QEMU to honor the SEV features specified in the IGVM file embedded in the VMSA. I recognize that this may take some doing, but is really the only robust way to ensure that the IGVM file gets the behaviors it expects at launch. This will, of course, become non-negotiable once we reach a point that the IGVM file includes an ID BLOCK that specifies the expected measurement of the SVSM image, which will contain the VMSA and its expected SEV features.

If this is too much to bite off in the short term, then it will be better for this PR to wait until that work is done. If there is strong desire to merge this PR before that work can be accomplished, then it would be much better for somebody else to pick up this PR and complete it - somebody who is able to actually work with QEMU and validate the workflow, which is something I am not going to be in a position to do.

roy-hopkins commented 9 months ago

I think a better strategy would be to modify QEMU to honor the SEV features specified in the IGVM file embedded in the VMSA.

Yes, and I've been working this week on exactly this. It will be fairly easy to set restricted injection to match the IGVM file in QEMU but due to the way this works in QEMU/kvm other features will require further work.

However, the current versions of QEMU we host in coconut currently require the extra command-line option to be set to enable restricted injection. If this PR is merged then users will have no option but to add that extra flag otherwise the SVSM will panic on startup, hence the suggestion for the documentation change in this PR.

But this is a draft PR, and if it is not going to be merged until we've moved to a QEMU that supports setting this via IGVM then we can leave the documentation unchanged.

msft-jlange commented 9 months ago

I think a better strategy would be to modify QEMU to honor the SEV features specified in the IGVM file embedded in the VMSA.

Yes, and I've been working this week on exactly this. It will be fairly easy to set restricted injection to match the IGVM file in QEMU but due to the way this works in QEMU/kvm other features will require further work.

However, the current versions of QEMU we host in coconut currently require the extra command-line option to be set to enable restricted injection. If this PR is merged then users will have no option but to add that extra flag otherwise the SVSM will panic on startup, hence the suggestion for the documentation change in this PR.

But this is a draft PR, and if it is not going to be merged until we've moved to a QEMU that supports setting this via IGVM then we can leave the documentation unchanged.

I think we are in agreement. This PR will remain a draft PR until your work to support VMSA-in-IGVM is complete. If there is pressure to move forward with requiring restricted injection before that, then we can revisit and talk about how to change the workflow (and document the changes).

msft-jlange commented 1 month ago

Both restricted injection and boot from IGVM are part of the SVSM patch set for KVM so this should be safe to merge now.

Freax13 commented 1 month ago

Both restricted injection and boot from IGVM are part of the SVSM patch set for KVM so this should be safe to merge now.

I'm assuming you're referring to https://github.com/coconut-svsm/linux/pull/7, right? That PR hasn't been merged yet and IMO we should wait for that. Otherwise, if users follow the installation guide in this repo, they'll end up with a broken setup.

msft-jlange commented 1 month ago

I'm assuming you're referring to coconut-svsm/linux#7, right? That PR hasn't been merged yet and IMO we should wait for that. Otherwise, if users follow the installation guide in this repo, they'll end up a broken setup.

@roy-hopkins has assured me that the existing SVSM patch set is sufficient to make this work correctly. Roy, did I misunderstand?

roy-hopkins commented 1 month ago

I'm assuming you're referring to coconut-svsm/linux#7, right? That PR hasn't been merged yet and IMO we should wait for that. Otherwise, if users follow the installation guide in this repo, they'll end up a broken setup.

@roy-hopkins has assured me that the existing SVSM patch set is sufficient to make this work correctly. Roy, did I misunderstand?

My error - I thought the COCONUT QEMU/KVM had been updated since this PR was opened, but the previous update to 6.10 was never merged and used as the default kernel. The comment I wrote above about updating documentation still stands for the current situation, but it makes more sense to wait for coconut-svsm/linux#7 to be merged. I'm working on that now.

msft-jlange commented 1 month ago

My error - I thought the COCONUT QEMU/KVM had been updated since this PR was opened, but the previous update to 6.10 was never merged and used as the default kernel. The comment I wrote above about updating documentation still stands for the current situation, but it makes more sense to wait for coconut-svsm/linux#7 to be merged. I'm working on that now.

Thanks for clarifying. I'll move this PR back to draft state and await your report on when it's safe to merge.