-
Notifications
You must be signed in to change notification settings - Fork 41
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.
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
Scrub memory before platform-specific invalidation #506
Conversation
Why is zeroing required at all when invalidating pages on TDP? The point of "invalidate" is to mark the pages as unusable prior to returning them to host control. But in the TDX architecture, there is no platform operation defined to do that. If the page is going to be used again, it must be accepted again, and the next call to TDG.MEM.PAGE.ACCEPT would fail if the host declined to retake control of the page as expected, and would zero it if the page went through the correct flow, so it doesn't seem like there's anything the L1 needs to do here. |
My guess is that we do this because Personally I'm not a big fan of this strategy. What happens if we try to accept memory that's already been accepted, but that also hasn't been zero-initialized yet? AFAICT we wouldn't get an error, but the memory wouldn't be zero-initialized. Is there a reason we zero-initialize the memory inside the guest instead of letting the TD module do it? |
In SVSM zeroing is mainly done to avoid leaking L1 information to L2. Since L1 and L2 share the same GPA space and L1 steals some of L2's lowmem for its own boot flow (e.g. stage1/stage2/kernel ELF/kernel fs), it makes sense to zero these pages out as part of |
I couldn't agree more. Ignoring
I failed to refactor adequately. The zeroing should be in the platform-specific implementation of |
I think "page invalidation" probably means something slightly different on TDP. I look forward to discussing with you what that means on TDP platforms. I'm open to addressing the concerns around |
Sorry, I think there's a slight misunderstanding here. I meant "TD module" as in the code running in SEAM root mode aka the code that's handling our TD calls aka the code signed by Intel, not "TD module" as in the module within the SVSM responsible for TDX stuff. Also, I just realized that it's actually called the TDX module, not the TD module, my bad. The zeroing already happens in a platform-specific module within the SVSM, but I was suggesting that we should rather let the
So far we understood "page invalidation" to refer to making memory inaccessible to the SVSM and guest. On SNP, this unsets the validate bit in the RMP, on TDP the equivalent would be to move the page back to the
I think "page invalidation" is slightly different on TDP because there's no TD call for a guest to request for a page to be moved back to the If we assume that well-behaved hypervisors always faithfully execute |
Hmm, so I just took a deeper look into the |
This is an interesting idea. I've not seen a TD guest do private->shared->private conversions for the sole purpose of moving it to On the topic of page invalidation, I'm curious as to why we would want to make SVSM's early boot memory regions inaccessible to the guest on TDP platforms. We support L2 guests that do not accept memory so the decision to accept a page is upon L1 itself. Also, our goal is to avoid giving the L2 guest a memory map that has fragmented lowmem. L2 BIOSes (e.g. OVMF) can assume that this range is contiguous during early memory discovery and we try to avoid adding special logic to them.
Yes, it's reasonable to assume that successful private->shared conversions always result in the removal of the private pages. I think behaviors breaking this assumption should be considered a bug. In the future we plan to introduce a "page acceptance bitmap" which tracks the pages that have already been accepted. We can then treat |
My main problem with just zeroing the page is that it doesn't match the doc-comment on the method:
I get your argument that we don't gain much from doing a private->shared->private conversion, but I also think that we should implement methods as they are documented. If the requirements don't make sense for a given platform maybe that's a sign that we should reconsider our abstractions. We currently have this function because on SNP calling pvalidate on a page that has already been pvalidate'd is very dangerous and we can't rely on an error like
Maybe we don't. If OVMF is fine with already-accepted memory, I don't see a reason why we need to make it inaccessible. We currently need this on SNP, but if OVMF on TDP is fine without this maybe we don't need to do it.
The COCONUT SVSM doesn't currently support that (i.e. unenlightened guests). This is one of the future goals, but as of right now, this is not supported on any platform.
👍
This bitmap already exists for SNP in the SVSM and I think maybe we can reuse it on TDP. The bitmap isn't currently part of any platform-specific abstraction, so maybe we don't even need to change anything? |
This thread has gotten a bit contorted, so I'll make a couple of points here. Nothing in COCONUT-SVSM relies on the property that accessing invalidated pages will fault. Nothing in any L1 should ever rely on that property. PVALIDATE(FALSE) exists on SNP to prevent the possibility of page substitution attacks, but that's an artifact of how SNP works. There is no analogous attack on TDX, so there is no reason to have the logical equivalent of PVALIDATE(FALSE). It is true that we need the ability to revoke access from an L2 so we can have the property that unexpected access by an L2 will generate a fault. This is needed for any unenlightened code. But we can achieve that property by simply revoking L2 access, which is possible on both SNP and TDX (on SNP, it is achieved through RMPADJUST and on TDX, it's achieved through TDG.MEM.PAGE.ATTR.WR). This revocation is not related to page invalidation. It is definitely the case that it is not possible to rely on Regardless of whether |
I am equally nervous about the safety of that crate and I had reached the same conclusion. There is very little special logic in that crate and replicating it (more robustly) in our own project is trivial and would be very worthwhile. |
That's pretty much the plan - to reuse that bitmap for TDP platforms as well. I'm glad we're on the same page :-) Thanks for explaining why page invalidation is needed on SNP. I believe there's no such hard requirement for TDP machines. OVMF does assume that some of the pages (such as pages needed by OVMF initialization code) have already been installed when it runs and it can discover them. Sounds like we're trying to make these interfaces meaningful for both platforms and I completely agree that the implementation in this PR doesn't match the doc-comment on the method. |
Yes this is still a goal of mine and the initial conversation with our OVMF owner was positive. OMVF now has incredibly complicated x86 boot paths because of platform-dependent requirements and perhaps this could be a step toward convergence. We're also working through some challenges we encountered when enabling OVMF for L2 TDP guests with them. My next step is to work with them and get back to you with their feedback. |
Yes it has some soundness/correctness issues overall. We were trying to not reinvent the wheel but we may have much higher security requirements than them. How about we open an issue for this rewrite and address it separately? |
Thanks for all of the feedback. It is clear now that there's no need for page invalidation on TDP platforms. I will try to address the concerns around page invalidation and update this PR. The goal is to remove any implication that zeroing these pages is equivalent to invalidation. |
Add align_down() in addition to align_up() to the Address trait, and use the generic align_{up,down}() calls while at it. Signed-off-by: Peter Fang <[email protected]>
1613746
to
f18f8cd
Compare
Updated the PR based on feedback |
Add page_align() in addition to page_align_up(). This mirrors the methods provided by the Address trait. Signed-off-by: Peter Fang <[email protected]>
Allow the entire virtual range to be allocated at once. No guard page is needed in this case. Signed-off-by: Peter Fang <[email protected]>
There's no need for page invalidation on TDP platforms and zeroing memory instead makes it confusing. Remove the implementation altogether. Signed-off-by: Peter Fang <[email protected]>
Page invalidation is not required on TDP platforms. Neutralize the naming so that these operations don't always imply invalidation. Memory scrubbing will be introduced in a later commit. No functional changes. Signed-off-by: Peter Fang <[email protected]>
Instead of invalidating memory, TDP platforms only need to zero out its content before handing it over to the L2 guest. This is not equivalent to page invalidation because the page remains accepted after zeroing. Always scrub the memory region prior to performing platform-specific invalidation (which is a no-op on TDP platforms). This should have no functional impact on SNP platforms. To scrub a physical memory region, the following strategies are adopted: - Add a cap to the mapping size per iteration - Try using 4K temporary mappings when the mapping size is small enough (< 2M) and try using 2M temporary mappings otherwise - When a mapping guard cannot be created, reduce the mapping size by half Signed-off-by: Peter Fang <[email protected]>
f18f8cd
to
b571616
Compare
I still don't understand the point of this PR. You're zeroing memory before it is invalidated. However, before it can be used again, it has to be validated again, and the process of validating it again will also zero it. So what's the point of zeroing at invalidation time? It looks like you are guarding against the possibility of leaking data from L1 to L2. However, in the SNP case, the pages are zeroed at the time they are validated by L2. In the TDP case, there is no code yet that knows how to manage an L2 at all, so it's way too premature to be making assumptions about how L1/L2 memory delegation for these specific memory ranges is supposed to work. It would be much better to defer all of this until there's a clear design for how memory delegation is supposed to work on all platforms and then we can make it work correctly on all of them. |
Makes sense. Dropping this PR and will be removing the page invalidation implementation for TDP in a different PR. |
Thanks for working through this! I'm planning on submitting a PR that removes references to |
Instead of invalidating memory, TDP platforms only need to zero out its content before handing it over to the L2 guest. This is not equivalent to page invalidation because the page remains accepted after zeroing.
Always scrub the memory region prior to performing platform-specific invalidation (which is a no-op on TDP platforms). This should have no functional impact on SNP platforms.