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

Safety properties? #3

Open
asajeffrey opened this issue Nov 5, 2019 · 11 comments
Open

Safety properties? #3

asajeffrey opened this issue Nov 5, 2019 · 11 comments

Comments

@asajeffrey
Copy link

It's not obvious what the safety properties of SharedSync are expected to be. In particular, if T is SharedSync, is it safe to take an &T into shared memory? If so, what is the attacker model, can the attacker write into the shared memory? Can they truncate the shared memory?

@asajeffrey
Copy link
Author

The equivalent trait in shared-data is SharedMemRef, described in https://docs.rs/shared-data/0.0.1/shared_data/trait.SharedMemRef.html but I think I'm dealing with a different attacker model, in particular allowing attackers to write to shared memory (but not truncate it, c.f. asajeffrey/shared-data#7). This means that &usize isn't safe to take into shared memory, as Rust assumes that the data pointed to by &usize is immutable.

@nagisa
Copy link
Collaborator

nagisa commented Nov 6, 2019

We did not consider the problem of peers being malignant when working on our shared memory use-cases as all the code we cared about was controlled by us in its entirety.

The traits in this crate presume that mutability properties of certain data are maintained and so &T would not change from under your feet if T has no interior mutability. This being shared memory, one cannot really take a &mut T either. This means that Ts for most practical purposes end up containing interior mutability anyway, usually in form of Atomic* types, and bare usize explicitly models some sort of immutable data that no peer should modify. In application code this might be modeled by marking certain pages of memory read-only.

That being said, my memory of this crate is fairly dry by this point, @Ekleog or @lovesegfault might have something else to add.

@lovesegfault
Copy link
Collaborator

lovesegfault commented Nov 6, 2019

IIRC we used to make it so that readers had RO access, cc. @Ekleog.

Also, @nagisa, we should really open source shmem, I'll do it while sick.

EDIT: Leo did it! https://github.com/standard-ai/shmem
@asajeffrey ^

@nagisa
Copy link
Collaborator

nagisa commented Nov 6, 2019

IIRC we used to make it so that readers had RO access

That is no longer true, and doesn’t really solve any of the issues raised by the OP in the first place AFAICT, as writer would still be able to invalidate compiler’s notion of &T being immutable by concurrent writing.

@lovesegfault
Copy link
Collaborator

Yeah, absolutely. I think if your writers are evil than you're always going to be fucked, no?

@asajeffrey
Copy link
Author

Well shared-data tries to be safe even in the presence of malicious writes; malicious truncation of the shared memory file is something you can't do much about. rust-lang/unsafe-code-guidelines#215 has a lot of discussion about what Rust can assume in terms of its run-time environement, and rust-lang/unsafe-code-guidelines#152 is a discussion of volatile read of untrusted memory (the issue there being that LLVM goes and treats that as potentially being UB, and Rust inherits that from LLVM, sigh).

There are some types that are safe to take references to, even in the presence of malicious writers. The poster child is &AtomicUsize which is safe to use no matter what writes the attacker does (OK, as long as you restrict the attacker so they can't write poison).

Woo re https://github.com/standard-ai/shmem

@Ekleog
Copy link
Collaborator

Ekleog commented Nov 7, 2019

@asajeffrey In this crate, we are considering all the processes having access to the shared memory region as being bound by the rust type system, and as respecting the invariants in the unsafe functions that can be called -- in this way, they are assumed to be adverse actors but bound by the type system and invariants of the functions.

The aim is to allow for fast cross-process communication, for which volatile read/writes would be impossible.

Also, volatile read/writes are not enough to prevent all UB, and you would actually need de/serializing to protect the stuff properly -- for instance, as far as I understand your code, Volatile<NonZero> is a legitimate type, and could actually evaluate to zero against a malicious opponent, which is UB.

That being said, I think I would see a point in having a trait for “this type is safe to put in shared memory in a purely adversarial context (ie. the opponent has full memory access) and to take a reference to it,” but I am not sure it could reasonably apply to anything due to the truncation issue. Do you have cases where you're 100% sure to be doing the right thing? Even &AtomicUsize might see its file truncated, which would end up in a SIGBUS, I think -- and as far as I know this is not a valid behavior for safe code, though I guess that's debatable?

As for where the traits should live, to be honest, I don't really think the most basic traits to interprocess communication should have any dependency or any code besides the most minimal one, as they ideally would end up in libstd at some point -- hence our creating a separate library for just this purpose.

@asajeffrey
Copy link
Author

Yes, our use-case is slightly different, it's the inter-process communication in Servo, where different processes correspond to different security domains. Ideally even if an attacker compromised one of the security domains, they wouldn't be able to compromise the others, which is why we're worried about attackers being able to write into shared memory. We're also concerned about attackers being able to truncate shared memory, but there's not much we can do about that on non-Linux OSs.

NonZero doesn't implement SharedMemCast or SharedMemSync so Volatile<NonZero> isn't a useful type.

Truncation is a real pain, ignoring it there are perfectly good types that are SharedMemCast, e.g. usize. Essentially SharedMemCast is types where every bitstring of the right length is a valid value, and SharedMemSync is the same, but with atomic access.

@Ekleog
Copy link
Collaborator

Ekleog commented Nov 8, 2019

Looks like I had misunderstood your code, then, thank you for the correction.

I guess the biggest issue I am having with SharedMemCast and SharedMemSync is that I can't really see them being implemented wider than a few built-in types, as any kind of struct or enum without repr already potentially embeds padding and thus is not either of those traits. (BTW, I see impl<T1: SharedMemRef> SharedMemRef for (T1,), are tuples' memory representation stable yet? I thought not).

As such, I'd expect any communication with an adversary to happen over a channel that transmits raw bytes, and to happen with serializing and deserializing, so that errors are handled properly anyway -- having the channel be implemented with volatile writes/reads on memory would then just be an implementation detail.

In case that's the way you want to go, there's a crate we should be open-sourcing soon, that provides a ringbuffer in shared memory. Its threat model was also designed with cooperative processes in mind, but off the top of my head it should not lead to UB with an adversary -- assuming a few additions to the API are made, to return a pointer instead of a reference at a few places, and to allow pushing multiple values at once for optimization purposes if you're going the RingBuffer<u8> way (as we are doing cooperative processes, we just push bigger structures, so that's not an immediate requirement for us).

@asajeffrey
Copy link
Author

@Ekleog For structs, I was hoping to be able to derive SharedMemSync in the same way as for SharedMemCast. I don't think Rust requires padding to be immutable, so something like (AtomicU8, AtomicUsize) should be fine, even though the padding is in volatile memory. I guess the unsafe code guidelines team might have something to say, I'll post a comment on rust-lang/unsafe-code-guidelines#152.

For enums I wote some notes at https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=646c26558c0844392716497ba6c549e6, the rough idea being to only support #[repr(C,u8)] enums and even then using a limited API.

Part of the aim here is to avoid the serialization/deserialization that ipc-channel currently uses, and provide zero-copy ipc channels. Servo currently uses shared memory for byte arrays as a special case to move images around without copying, it would be nice if that wasn't a special case any more.

Your ring buffer looks really interesting, you can see the first shot I made at channels at https://github.com/asajeffrey/shared-data/blob/master/src/shared_channel.rs, the main difference AFAICT is that Servo needs unbounded channels (our deadlock-freedom properties depend on that).

@jean-airoldie
Copy link

jean-airoldie commented Nov 23, 2019

@asajeffrey What about passing types marked by the trait StableABI? The lib is a little on the heavy side, but essentially you can #[derive(StableAbi)] for anything that is repr(C) or a primitive. This would allows you to directly pass types across processes without needing to serialize.

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

5 participants