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

The map_to function should be unsafe #134

Closed
phil-opp opened this issue Mar 6, 2020 · 3 comments · Fixed by #135
Closed

The map_to function should be unsafe #134

phil-opp opened this issue Mar 6, 2020 · 3 comments · Fixed by #135
Labels
Milestone

Comments

@phil-opp
Copy link
Member

phil-opp commented Mar 6, 2020

The Mapper::map_to method used to be unsafe because it is possible to break memory safety by pointing to unrelated pages to the same physical frame. In #89, we made the function safe by introducing a new UnusedPhysFrame wrapper type that is unsafe to construct.

In retrospect, I think this was a mistake because it is now possible to break memory safety using only the safe methods the Mapper trait. For example:

  • Call unmap for some page and then map the page to a different UnusedPhysFrame. Now we invalidated all values that are stored in this page.
  • Call map with the GLOBAL and MUTABLE flags. This crates a mapping that is not flushed from the TLB on address space switches, so that other processes can manipulate the data of the page.
    • (Technically, such a mapping alone cannot lead to undefined behavior since unsafe code is required in order to use the new mapping (e.g. for converting the page address to a reference type). However, I still think that we should treat this operation as unsafe given that it is a clear footgun.)

Given the complexity of page table mappings, I think the best solution to this problem is to treat the map_to method as a fundamentally unsafe operation. This means that we should not try to make it "less unsafe" by introducing wrapper types like UnusedPhysFrame because there will probably always be a way to still do something unsafe.

So I propose to revert #89, thereby making map_to unsafe again. Instead of completely removing the UnusedPhysFrame type, we could deprecate it to reduce breakage and make its new function return a PhysFrame, so that it can still be used with map_to. This way, we would also resolve #133 and #123.

@phil-opp
Copy link
Member Author

phil-opp commented Mar 6, 2020

cc @gnzlbg (originally opened #88)

@phil-opp
Copy link
Member Author

phil-opp commented Mar 6, 2020

WIP implementation in #135.

@phil-opp
Copy link
Member Author

phil-opp commented Mar 6, 2020

I think the update_flags method should be unsafe too, given that it can make existing mappings e.g. mutable or global.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant