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

Unsoundness: VolatileMemory should be unsafe, private, or sealed because it relies on get_slice in unsafe code #250

Closed
Manishearth opened this issue Aug 17, 2023 · 3 comments

Comments

@Manishearth
Copy link

/// Returns a [`VolatileSlice`](struct.VolatileSlice.html) of `count` bytes starting at
/// `offset`.
fn get_slice(&self, offset: usize, count: usize) -> Result<VolatileSlice<BS<Self::B>>>;

The behavior of this trait method is relied upon in multiple places in unsafe code, e.g:

// SAFETY: This is safe because the pointer is range-checked by get_slice, and
// the lifetime is the same as self.

A user of this library implementing VolatileMemory could cause unsoundness by writing an incorrect implementation of VolatileMemory::get_slice(). This means that it should be an unsafe trait (or private/sealed) with the safety requirements for implementors documented on the trait.

@roypat
Copy link
Collaborator

roypat commented Aug 21, 2023

Thank you for bringing your security concerns to our attention! We will investigate these immediately and follow up with you within 5 business days to provide a status.

If you have any further questions or concerns, please do not hesitate to contact us according to the rust-vmm security disclosure policy.

Thanks again!

@Manishearth
Copy link
Author

Manishearth commented Aug 21, 2023

Thanks! I'm not in a rush.

(I did consider using the security policy but didn't in the end since this is basically "someone can cause UB if they use your library weirdly" which is bad from a rust unsafe model perspective but basically ok from a security perspective.)

@roypat
Copy link
Collaborator

roypat commented Aug 29, 2023

Hello @Manishearth,
we have fixed the issue in #251 and published vm-memory 0.12.2 to crates.io. We will also go through the process of filing a RustSec advisory for affected versions. Thank you again for reporting this issue!

@roypat roypat closed this as completed Aug 29, 2023
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

No branches or pull requests

2 participants