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.

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

Surround memory mappings with guard pages #77

Closed
wants to merge 1 commit into from
Closed

Surround memory mappings with guard pages #77

wants to merge 1 commit into from

Conversation

serban300
Copy link

@serban300 serban300 commented Feb 20, 2020

implements #62

In theory it is possible to take a pointer to a MmapedRegion and read/write outside of the region's bounds. We can try to prevent this behavior by surrounding the underlying mapped memory with guard pages.

Posting this as a draft in order to understand if this feature is of any interest to the community and to the maintainers of the crate.

In theory it is possible to take a pointer to a `MmapedRegion` and
read/write outside of the region's bounds. We try to prevent this
behavior by surrounding the underlying mapped memory with guard pages.

Signed-off-by: Serban Iorga <[email protected]>
@serban300
Copy link
Author

@bonzini @jiangliu @alexandruag

Sorry, just a quick ping. Can you take a look on this proposal ?

Thanks !

@jiangliu
Copy link
Member

@bonzini @jiangliu @alexandruag

Sorry, just a quick ping. Can you take a look on this proposal ?

Thanks !

Will these red zones cause trouble to huge page mappings?

@serban300
Copy link
Author

Will these red zones cause trouble to huge page mappings?

@jiangliu I don't think huge page mappings can lead to corner cases here. So I can't think of any trouble. I hope I'm not missing anything.

@serban300
Copy link
Author

Hi @bonzini, @jiangliu, @alexandruag,

When you have a few minutes to spare could you help me with a quick high-level opinion here ?

I would just like to understand if there is any interest in this feature or if you think that it would bring enough value compared to the added complexity in order to pursue it. This PR is only a draft, but if there's any interest I can spend some more time and try to clean up the implementation. If not, I can just close it.

Thanks,
Serban

@glitzflitz
Copy link

Have you considered the case of jumping over the guard pages? Although the possibility is less due to correct bound checking but there were vulnerabilities related to this in linux kernel. Even though stack based arrays aren't widely used in rust but I'm not sure if rust has equivalent of GCC's "-fstack-check" option just for the extra precaution.
https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt
https://lkml.org/lkml/2017/6/22/345
One option is to enable configuring the number of pages that are used as guards.

return false;
}

let page_size = match unsafe { libc::sysconf(libc::_SC_PAGESIZE) } {
Copy link
Member

Choose a reason for hiding this comment

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

Should we align the redzone to hugepage size(2M/1G)?
Otherwise it may conflicts with THP.

@jiangliu
Copy link
Member

Could you please help add some unit tests for the new interface?

@serban300
Copy link
Author

@glitzflitz thanks for the comment. It's true that this protection doesn't cover all the scenarios. It's always possible to jump over the guard pages. But it makes it less likely to break out of the memory region especially by accident. I am not very familiar with -fstack-check. But these memory regions are allocated on the heap. So I'm not sure if it would help.

@jiangliu thank you for the review. As soon as I can I will take another look on this draft PR, try to simplify it if possible, and also address your comments.

@alexandruag
Copy link
Collaborator

Hi, apologies for the delayed response. I'll have a better look at the code a bit later, but right now it seems like a less invasive option is to leverage the build_raw constructor for MmapRegion introduced in #109 (not published yet, but we can do that soon) + externally managed mappings.

My current preference is to see what can be done in terms of simplications/performance improvements before changing how other things work (for example, if it makes sense to restrict the host addresses of the mapped regions to get contiguous areas, that might simplify using guard pages as well). Does the build_raw-based approach sound reasonable for the time being?

@serban300
Copy link
Author

@alexandruag Thanks for the suggestion. If I understand correctly build_raw looks like a good fit. I'll dive a bit deeper when I have time.

@serban300
Copy link
Author

Hey ! As suggested by @alexandruag after a new version of vm-memory is released, I will use build_raw in order to create custom mmap regions in Firecracker surrounded by guard pages. I will close this PR for the moment.

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.

4 participants