Skip to content

Conversation

@roypat
Copy link
Contributor

@roypat roypat commented Apr 2, 2025

Changes

Add a secret_free parameter to the API, which indicates to Firecracker that non-swiotlb memory should be backed by guest_memfd, and that this guest_memfd should be initialized in a way such that its direct map entries are zapped.

Reason

...

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 69.18429% with 102 lines in your changes missing coverage. Please review.

Project coverage is 82.73%. Comparing base (71e33a0) to head (df1780d).
Report is 13 commits behind head on feature/secret-hiding.

Files with missing lines Patch % Lines
src/vmm/src/vstate/vm.rs 54.76% 38 Missing ⚠️
src/vmm/src/resources.rs 58.62% 36 Missing ⚠️
src/vmm/src/builder.rs 54.34% 21 Missing ⚠️
src/vmm/src/vstate/memory.rs 91.46% 7 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                    @@
##           feature/secret-hiding    #5131      +/-   ##
=========================================================
- Coverage                  82.86%   82.73%   -0.13%     
=========================================================
  Files                        251      251              
  Lines                      27291    27522     +231     
=========================================================
+ Hits                       22614    22771     +157     
- Misses                      4677     4751      +74     
Flag Coverage Δ
5.10-c5n.metal 83.20% <66.05%> (-0.18%) ⬇️
5.10-m5n.metal 83.20% <66.05%> (-0.18%) ⬇️
5.10-m6a.metal 82.37% <66.05%> (-0.18%) ⬇️
5.10-m6g.metal 79.07% <65.84%> (-0.16%) ⬇️
5.10-m6i.metal 83.19% <66.05%> (-0.19%) ⬇️
5.10-m7a.metal-48xl 82.37% <66.05%> (-0.17%) ⬇️
5.10-m7g.metal 79.07% <65.84%> (-0.16%) ⬇️
6.1-c5n.metal 83.24% <66.05%> (-0.17%) ⬇️
6.1-m5n.metal 83.24% <66.05%> (-0.17%) ⬇️
6.1-m6a.metal 82.41% <66.05%> (-0.18%) ⬇️
6.1-m6g.metal 79.07% <65.84%> (-0.16%) ⬇️
6.1-m6i.metal 83.24% <66.05%> (-0.18%) ⬇️
6.1-m7a.metal-48xl 82.41% <66.05%> (-0.18%) ⬇️
6.1-m7g.metal 79.07% <65.84%> (-0.16%) ⬇️
6.14-c5n.metal 83.20% <63.60%> (-0.22%) ⬇️
6.14-m5n.metal 83.21% <63.60%> (-0.21%) ⬇️
6.14-m6a.metal 82.39% <63.60%> (-0.20%) ⬇️
6.14-m6g.metal 79.03% <63.38%> (-0.19%) ⬇️
6.14-m6i.metal 83.20% <63.60%> (-0.22%) ⬇️
6.14-m7a.metal-48xl 82.38% <63.60%> (-0.21%) ⬇️
6.14-m7g.metal 79.03% <63.38%> (-0.19%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roypat roypat force-pushed the secret-freedom-guest-memfd branch 8 times, most recently from cf8e81b to e593aa2 Compare April 2, 2025 14:14
@roypat roypat force-pushed the feature/secret-hiding branch from bf38d65 to cfd4d8b Compare April 3, 2025 05:39
@roypat roypat force-pushed the secret-freedom-guest-memfd branch 10 times, most recently from 80824bf to 41f5cb7 Compare April 8, 2025 14:12
roypat added 2 commits April 8, 2025 15:57
Add a utility function for creating a guest_memfd and wrapping it into a
`File` object.

Signed-off-by: Patrick Roy <[email protected]>
There's be a lot more things that are incompatible going forward (mostly
related to secret freedom), so instead of adding a ton of error variants
for each pair of incompatible features, let's just have a single one
where we can insert arbitrary features via a string argument.

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat force-pushed the secret-freedom-guest-memfd branch from ceaf67f to 80411d7 Compare April 8, 2025 14:57
@roypat roypat changed the title [WIP] Use guest_memfd for backing "secret free" VMs Use direct map removed guest_memfd for backing "secret free" VMs Apr 8, 2025
@roypat roypat marked this pull request as ready for review April 8, 2025 14:59
@roypat roypat force-pushed the secret-freedom-guest-memfd branch 2 times, most recently from 476c68c to 6059353 Compare April 8, 2025 15:43
This will later indicate to Firecracker that guest memory should be
backed by guest_memfd.

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat force-pushed the secret-freedom-guest-memfd branch from a8c1367 to 405f22c Compare April 9, 2025 10:30
@roypat roypat force-pushed the secret-freedom-guest-memfd branch 2 times, most recently from 75b4399 to dd61db6 Compare April 9, 2025 11:59
@roypat roypat force-pushed the secret-freedom-guest-memfd branch from dd61db6 to 5579e7a Compare April 9, 2025 13:08
roypat added 10 commits April 9, 2025 14:12
When guest memory is backed by direct map removed guest_memfd ("secret
free"), we cannot load the guest kernel / initrd by read-ing into guest
memory, as the kernel won't be able to access guest memory. Instead
we'll need to read into a userspace buffer, and then memcpy into guest
memory. Add a newtype that wraps Read/Write and performs exactly this
operation, and use it to load guest kernel / initrd.

Signed-off-by: Patrick Roy <[email protected]>
Fall back to kvm_user_memory_region in case the 2 version of the struct
isnt supported.

Signed-off-by: Patrick Roy <[email protected]>
vm-memory has faulty validation logic that prevents us from mmap-ing
guest_memfds, so just bypass that by calling mmap ourselves for the time
being.

See also rust-vmm/vm-memory#320

Signed-off-by: Patrick Roy <[email protected]>
If the `secret_free` field of the memory_config is set to true in the
/machine-config endpoint, back all non-swiotlb regions using
guest_memfd. For our setup, this means both setting the
guest_memfd[_offset] fields in kvm_user_memory_region2, as well as
mmaping the guest memory and reflecting this VMA back into the memslot's
userspace_addr (which is how kvm internal accesses to guest memory will
work for these guest_memfd regions, such as mmio emulation on x86).

Signed-off-by: Patrick Roy <[email protected]>
To take snapshots of secret hidden VMs, we need to bounce guest memory
through a userspace buffer. Reuse the `Bounce` wrapper type that is
already in use for loading the guest kernel / initrd.

Signed-off-by: Patrick Roy <[email protected]>
On x86, only KVM_X86_SW_PROTECTED_VM VMs support guest_memfd, so we have
to explicitly specify this VM type. On Arm, all VMs support it.

Signed-off-by: Patrick Roy <[email protected]>
The current version of the mmap-support patches require that on x86,
memory attributes have to be set to private even if the guest_memfd VMA
is short-circuited back into the memslot (on ARM, memory attributes are
not even supported in this scenario).

Signed-off-by: Patrick Roy <[email protected]>
Add a test that we can boot "normal" VMs on ARM with secret freedom
enabled (e.g. I/O works through the swiotlb region), and test that on
x86 we can boot at least an initrd (e.g. a very simple VM that doesnt
have any I/O devices attached).

Skip tets on m6g.metal, as currently direct map removal causes panics on
this hardware.

Signed-off-by: Patrick Roy <[email protected]>
Get some throughput data from our perf tests with secret freedom
enabled.

Signed-off-by: Patrick Roy <[email protected]>
Since we load the kernel using bounce buffers now, it will give us
false-positives.

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat force-pushed the secret-freedom-guest-memfd branch from 5579e7a to df1780d Compare April 9, 2025 13:12
@roypat roypat requested review from JackThomson2 and kalyazin April 9, 2025 14:30
@roypat roypat merged commit 1891d72 into firecracker-microvm:feature/secret-hiding Apr 9, 2025
5 of 7 checks passed
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