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.

Sign up for GitHub

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

Jump to bottom

Add test to check the pre-calculated launch measurement #392

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add test to check the pre-calculated launch measurement #392

wants to merge 6 commits into from

Conversation

stefano-garzarella
Copy link
Contributor

This PR introduces a test to check that the launch measurement pre-calculated by igvmmeasure is the same as that returned by the VMPL0 attestation report at runtime.
To support that, this PR also adds a serial port (COM3) to allow the host to communicate with the test running in the VM. This could be used in the future for other tests as well.

Tested locally with:

QEMU=/path/to/coconut/qemu/build/qemu-system-x86_64 make test-in-svsm

@roy-hopkins
Copy link
Collaborator

Just taking a look at this. At the moment I'm getting a test failure due to an incorrect measurement - probably due to my environment.

roy-hopkins
roy-hopkins approved these changes Jul 10, 2024
View reviewed changes
Copy link
Collaborator

@roy-hopkins roy-hopkins left a comment

Choose a reason for hiding this comment

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

This is great! It's already saved me lots of time in debugging attestation measurement differences.

Code reviewed and tested on Genoa system.

00xc
00xc reviewed Jul 10, 2024
View reviewed changes
kernel/src/greq/services.rs Outdated Show resolved Hide resolved
00xc
00xc reviewed Jul 10, 2024
View reviewed changes
kernel/src/greq/services.rs Outdated Show resolved Hide resolved
@stefano-garzarella stefano-garzarella marked this pull request as draft July 10, 2024 16:18
@stefano-garzarella
Copy link
Contributor Author

Pushed by mistake (I had to test the code on the SEV-SNP machine) and there is something wrong with the serial port locks by moving them to kernel/src/testing.rs

@stefano-garzarella
Copy link
Contributor Author

Strage, if I rebase on previous commit (126400b) it works, but on current main (fa92efc), the test are not executed and the output is just the Stage2:

[Stage2] COCONUT Secure Virtual Machine Service Module (SVSM) Stage 2 Loader
[Stage2] Order-00: total pages:    15 free pages:     1
[Stage2] Order-01: total pages:     0 free pages:     0
[Stage2] Order-02: total pages:     1 free pages:     1
[Stage2] Order-03: total pages:     1 free pages:     1
[Stage2] Order-04: total pages:     0 free pages:     0
[Stage2] Order-05: total pages:     0 free pages:     0
[Stage2] Total memory: 108KiB free memory: 52KiB
[Stage2]   kernel_region_phys_start = 0x0000008000000000
[Stage2]   kernel_region_phys_end   = 0x0000008001000000
[Stage2]   kernel_virtual_base   = 0xffffff8000000000

This happen only with the last patch of this PR applied. Any idea?

@stefano-garzarella
Copy link
Contributor Author

On current main, this PR works only if I revert commit 683ce95 and 79c18d6 , but I'm not sure if they are related.

Without reverting them, SVSM is not starting and all the tests are not executed.
Ideas or suggestions on how to debug? @roy-hopkins @00xc

@00xc
Copy link
Member

00xc commented Jul 11, 2024 •
edited

Interesting, when I run in-SVSM tests on your branch I get the following:

[SVSM] test greq::pld_report::tests::test_ecdsa_p384_sha384_signature_offsets ...
[SVSM] test greq::pld_report::tests::test_snp_report_request_offsets ...
[SVSM] test greq::pld_report::tests::test_snp_report_response_offsets ...
[SVSM] test greq::services::tests::test_snp_launch_measurement ...
[SVSM] ERROR: Panic: CPU[0] panicked at kernel/src/greq/services.rs:137:9:
assertion `left == right` failed
  left: [204, 126, 175, 255, 231, 55, 151, 54, 177, 236, 0, 166, 142, 10, 234, 39, 153, 46, 210, 245, 52, 168, 212, 128, 10, 166, 62, 87, 28, 45, 49, 82, 146, 129, 80, 12, 185, 94, 82, 58, 16, 248, 208, 72, 200, 24, 252, 208]
 right: [234, 244, 164, 113, 111, 124, 171, 218, 152, 54, 231, 36, 150, 137, 51, 7, 22, 58, 202, 168, 149, 134, 254, 136, 100, 43, 182, 202, 16, 192, 223, 85, 152, 218, 40, 209, 239, 169, 45, 90, 201, 173, 204, 254, 253, 14, 139, 38]
[SVSM] ---BACKTRACE---:
[SVSM]   [0xffffff80000f17ee]
[SVSM]   [0xffffff8000030d0d]
[SVSM]   [0xffffff800003d33e]
[SVSM]   [0xffffff800009dc61]
[SVSM]   [0xffffff800002b1c1]
[SVSM]   [0xffffff80000313a1]
[SVSM]   [0xffffff80000c8cb5]
[SVSM]   [0xffffff80000c4d5b]
[SVSM]   Invalid frame
[SVSM] ---END---

The output is very similar in release mode.

@stefano-garzarella
Copy link
Contributor Author

@00xc just to be sure, commit a9469ea ?

Now that you mention I tried to run the in-SVSM tests in release mode, and it worked (all tests passed included the new one), while in debug I still have #392 (comment)

@stefano-garzarella
Copy link
Contributor Author

BTW the mismatch could be related to the host kernel, (e.g. debug_swap kernel parameter). Do you have some param enabled or a different kernel in the host? (I'm using commit bc4de28e0cc1e7cf404af311c4052560aba355ba from coconut's linux)

@00xc
Copy link
Member

00xc commented Jul 11, 2024 •
edited

@00xc just to be sure, commit a9469ea ?

Yep.

Now that you mention I tried to run the in-SVSM tests in release mode, and it worked (all tests passed included the new one), while in debug I still have #392 (comment)

It works for me in both modes.

However, I noticed that we do the following when booting with IGVM:

svsm/Makefile

Line 77 in fa92efc

$(IGVMBUILDER) --sort --output $@ --tdx-stage1 bin/stage1-trampoline.bin --stage2 bin/stage2.bin --kernel bin/svsm-kernel.elf --comport 3 hyper-v --native --snp --tdp

The important part being --comport 3, which results in setting I/O port 0x3e8 (COM3) as the debug port. I noticed that you also use this port in your implementation. Could it be that test I/O and console I/O are interfering with each other?

Note that using qemu's fw_cfg instead uses port 0x3f8 (COM1).

@stefano-garzarella
Copy link
Contributor Author

I just did rustup update:

   stable-x86_64-unknown-linux-gnu updated - rustc 1.79.0 (129f3b996 2024-06-10) (from rustc 1.78.0 (9b00956e5 2024-04-29))
  nightly-x86_64-unknown-linux-gnu updated - rustc 1.81.0-nightly (0c81f94b9 2024-07-10) (from rustc 1.80.0-nightly (8387315ab 2024-05-14))

And now it's working also here, but now I'm not sure if we have some hidden issue.

@stefano-garzarella
Copy link
Contributor Author

However, I noticed that we do the following when booting with IGVM:

svsm/Makefile

Line 77 in fa92efc

$(IGVMBUILDER) --sort --output $@ --tdx-stage1 bin/stage1-trampoline.bin --stage2 bin/stage2.bin --kernel bin/svsm-kernel.elf --comport 3 hyper-v --native --snp --tdp

The important part being --comport 3, which results in setting I/O port 0x3e8 (COM3) as the debug port. I noticed that you also use this port in your implementation. Could it be that test I/O and console I/O are interfering with each other?

ehm, why we are setting --comport 3 ?

Note that using qemu's fw_cfg instead uses port 0x3f8 (COM1).

Without this PR, in scripts/test-in-svsm.sh we don't set the backend form COM3, so I'm not sure SVSM is really using it.

Should I use COM4?

@stefano-garzarella
Copy link
Contributor Author

Oh wait, --comport 3 is only for bin/coconut-hyperv.igvm, for QEMU we are using the default (1).

@00xc
Copy link
Member

00xc commented Jul 11, 2024

Oh wait, --comport 3 is only for bin/coconut-hyperv.igvm, for QEMU we are using the default (1).

Yes, that's what I meant :)

@00xc
Copy link
Member

00xc commented Jul 11, 2024 •
edited

Should I use COM4?

Yes, please try using a different port for tests.

@stefano-garzarella
Copy link
Contributor Author

Should I use COM4?

Yes, please try using a different port for tests.

Okay, I'll do.

About the test failure in your env, can you check this:

BTW the mismatch could be related to the host kernel, (e.g. debug_swap kernel parameter). Do you have some param enabled or a different kernel in the host? (I'm using commit bc4de28e0cc1e7cf404af311c4052560aba355ba from coconut's linux)

@stefano-garzarella
Copy link
Contributor Author

v3:

  • replaced COM3 with COM4 to avoid conflicts with hyper-v
  • added a new patch to refactor a bit the serial ports in scripts/launch_guest.sh

v2:

  • moved API to get the serial port used for tests in kernel/src/testing.rs
  • added enum for tests and documentation

@stefano-garzarella stefano-garzarella marked this pull request as ready for review July 11, 2024 11:10
@00xc
Copy link
Member

00xc commented Jul 11, 2024

BTW the mismatch could be related to the host kernel, (e.g. debug_swap kernel parameter). Do you have some param enabled or a different kernel in the host? (I'm using commit bc4de28e0cc1e7cf404af311c4052560aba355ba from coconut's linux)

There is no such parameter in the cmdline. I'm not sure which host version we have, as it is an internal machine on which I did not install the kernel. uname says "6.8.1-1-coco".

@roy-hopkins
Copy link
Collaborator

Apologies for the delay in joining this thread - I was away.

There is no such parameter in the cmdline. I'm not sure which host version we have, as it is an internal machine on which I did not install the kernel. uname says "6.8.1-1-coco".

I've just checked on that same machine - the 6.8.1-1-coco kernel has DEBUG_SWAP disabled which should match the IGVM file that is generated but I also see a mismatch in the attestation report. I can confirm however that your latest branch works on my SEV-SNP system for both debug and release builds and the attestation measurement matches.

@stefano-garzarella
Copy link
Contributor Author

v4:

  • rebased on 6126750
  • add $(IGVMMEASUREBIN) dependency to test-in-svsm target in the Makefile

@stefano-garzarella
Copy link
Contributor Author

@00xc about the problem you were seeing on your machine, could it be related to #433 ?

Should we merge this PR to discover that issues?

@joergroedel
Copy link
Member

I am also running in a measurement mismatch error, debug_swap is off on my machine. The machine has a Milan CPU, any hints on what might be wrong?

@Freax13
Copy link
Contributor

Freax13 commented Aug 22, 2024

This works for me. Tested on EPYC Milan. I'm using the latest KVM and QEMU patches mentioned in the docs.

@joergroedel
Copy link
Member

@stefano-garzarella Can you please downgrade a measurement mismatch from a fatal error to a warning? With that the PR is ready to merge and we can move on fixing the causes of the mismatches.

@stefano-garzarella
greq/pld_report: add measurement() API to SnpReportResponse
151bead 
Provide a method to obtain the measurement from the SNP
attestation report.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@stefano-garzarella
kernel/testing: add svsm_test_io() method
3317caa 
The first test calling the svsm_test_io() function will initialize the
serial port (COM4) and use it exclusively, since it will be protected
by LockGuard.

This serial port can be used in tests running in SVSM to communicate
with the host. The protocol is simple, guest writes 1 byte identifying
the request (IORequest), subsequent in/out depends on each request.

Next commit will extend scripts/test-in-svsm.sh to handle the requests
in the host side.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@stefano-garzarella
Copy link
Contributor Author

v5:

  • rebased on 1fb6f4f
  • added assert_eq_warn!() macro
  • call assert_eq_warn!() instead of assert_eq!() to avoid test failure till we fix the mismatch

@stefano-garzarella
Copy link
Contributor Author

I am also running in a measurement mismatch error, debug_swap is off on my machine. The machine has a Milan CPU, any hints on what might be wrong?

What kernel is running your host?

@stefano-garzarella Can you please downgrade a measurement mismatch from a fatal error to a warning? With that the PR is ready to merge and we can move on fixing the causes of the mismatches.

@joergroedel done ;-)

00xc
00xc reviewed Aug 27, 2024
View reviewed changes
kernel/src/testing.rs Outdated Show resolved Hide resolved
@stefano-garzarella
kernel/testing: add assert_eq_warn!() macro
98cafb5 
It is similar to the assert_eq!() macro, but instead of panicking,
it prints a warning message with the same format.

This new macro is useful for those tests that might fail and need
to be fixed.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@stefano-garzarella
scripts/launch_guest.sh: add variables to keep track of COM*
749bcc4 
In QEMU the order of the `-serial` parameters defines the I/O port to be
used (COM*), so let's add variables to keep track of them and set them
with `null` backend when not used to preserve the order regardless of
the script parameters.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@stefano-garzarella
scripts: add serial port to communicate with tests running in SVSM
9678187 
Use a serial port (COM4) with QEMU when we run unit tests in SVSM.
This way we can use it to communicate with tests running in the guest.

The serial port is connected to 2 pipes used for IN and OUT.
Pipes are created in a temporary folder and destroyed when
testing is completed.

The guest sends a request (1 byte) and the host responds according
to the type of the request. See the next commit for an example.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
@stefano-garzarella
greq/services: add test to check the pre-calculated launch measurement
980e346 
Ask the host for the pre-calculated launch measurement, and ask for a
VMPL0 attestation report and compare them. This way we can avoid
regressions in our tools for calculating the expected launch
measurement.

Since we still have some cases where the precalculated value does not
match, for now we just issue a warning until we fix the problem.

`scripts/test-in-svsm.sh` is now using `bin/igvmmeasure`, so let's
add `$(IGVMMEASUREBIN)` dependency to `test-in-svsm` target in the
Makefile.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@00xc 00xc 00xc left review comments

@roy-hopkins roy-hopkins roy-hopkins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stefano-garzarella @roy-hopkins @00xc @joergroedel @Freax13