Repository navigation
Conversation
| } | ||
| ``` | ||
|
|
||
| However, this would require a new trait in the standard library, so this might |
There was a problem hiding this comment.
Hello. Yes, I'd merge this PR for sure.
|
|
||
| However, until LLVM adds support for this intrinsic, the compiler can generate a | ||
| call to a function that performs the copy, or a call to `memcpy()` (if LLVM can | ||
| be persuaded not to treat this as equivalent to the `llvm.memcpy` intrinsic). |
There was a problem hiding this comment.
Cc @nikic -- do you think it makes sense to add a flag to LLVM's memcpy that indicates a freeze? Ideally we'd not have to hand-roll our own freezing memcpy in Rust, that seems a bit silly. We want to use the regular highly-optimized memcpy primitive, just in a way that LLVM considers to be freezing all undef/poison.
| would also be consistent with the existing `zeroed()` method. | ||
|
|
||
| However, the name `freeze` for this operation is already well established by the | ||
| LLVM instruction, so this RFC proposes to use `freeze` instead of `frozen`. |
There was a problem hiding this comment.
I don't find this very convincing. We don't usually name things after the LLVM IR operation they compile to.
I think it should be called frozen.
| The biggest disadvantage of adding the `freeze` operation is that **a Rust | ||
| program can leak a secret that was previously stored in the uninitialized memory | ||
| without triggering UB**. |
There was a problem hiding this comment.
In general, we don't have to make things UB to consider them wrong and try to prevent them. We also have the concept of erroneous behavior (EB), where the program is considered buggy and can be aborted if e.g. run under Miri or a sanitizer, but otherwise has well-defined behavior (or at least not UB).
Could we have EB for at least some subset of Rust programs that leak uninitialized memory? It makes no sense to consider every use of freeze that sees an uninit byte to be EB, if we want that then we shouldn't add freeze to begin with. But tools like Valgrind and MemorySanitizer can diagnose specific uses of uninitialized memory that are likely bugs, e.g., branching on a condition or dereferencing a pointer derived from uninitialized memory.
Is there a way to specify EB that blesses (roughly) the kind of checks those tools perform? That seems like it would provide a decent compromise (it rules out use case 1 but still allows other use cases). Unfortunately I don't see an easy way to do it:
- A simple operational semantics would be a naive "taint tracking" model where bytes that were uninit and got frozen are still recorded as being "tainted" by uninit-ness, and this is propagated though essentially every operation on values, and certain operations on tainted values/memory (e.g., branching or outputting) are EB. However, this disallows use case 2 where the offending bits are masked out, and likely other use cases as well.
- To be smarter about when the "taint" of uninit-ness can be safely considered defused, one could try to do "possible values of non-det choice" reasoning like LLVM's
undef(e.g.,undef & 1is either 0 or 1 and(undef & 1) >> 1is always 0). However, this seems very hard to reason about and and possibly makes some desirable compiler optimizations illegal (w.r.t. not introducing EB). - A more teleological definition would be that there is EB if the observable behavior of the program depends on the non-deterministic choices made by
freezeoperations. However, this is impossible to implement, and allows some programs that sanitizers will flag as using uninitialized memory.
There was a problem hiding this comment.
"program leaks secrets" is not something that you can define as a property of an AM execution, so I don't think we can have Miri detect this or call it EB. (Formally it's a hyperproperty, you need to define a notion of "public"/"secret" data and then compare two runs of the program to determine that a secret was leaked.)
A simple operational semantics would be a naive "taint tracking" model where bytes that were uninit and got frozen are still recorded as being "tainted" by uninit-ness,
I think you are inventing provenance for integers. Please, let's not.
There was a problem hiding this comment.
I'm well aware that "leaks secrets" can't be operationalized at AM level, and of other challenges. I'm wondering whether there is some property of an AM execution that we can define, which is somewhat related to improper use of uninitialized memory, and useful as EB: doesn't rule out any important use cases, but diagnoses some obviously buggy programs. Tools like Valgrind and MemorySanitizer are useful and already don't complain about some of the things that freeze would allow doing. It would be a shame if we had to essentially turn them off completely around any use of freeze.
Maybe the fact that freeze is opt-in is good enough to still catch all the same bugs in practice. But it's not obvious to me.
There was a problem hiding this comment.
It would be a shame if we had to essentially turn them off completely around any use of freeze.
I can't think of anything better. Any way of distinguishing the result of freeze from a normal integer amounts to essentially a form of provenance on integers, and that's too big a hammer for this IMO.
There was a problem hiding this comment.
imo whatever we pick should support stuff like Atomic::<(u8, u16)>::compare_exchange which needs to be able to freeze the padding bytes (probably using MaybeUninit<[u8; 4]>) and have them turn into normal bytes that don't report errors because you had to compare them in your cmpxchg loop.
There was a problem hiding this comment.
Please let's not scope creep. Atomic on types with padding has a bunch of extra complications.
There was a problem hiding this comment.
I'm not saying Rust should implement Atomic for types with padding, but that with freeze a user library could.
There was a problem hiding this comment.
Ah I see. Yeah that may be possible, but one has to be careful in from_mut (it probably has to freeze padding).
|
Looks like all the first-round feedback was handled. Let's nominate this for t-lang. |
|
To be honest, I am now even less convinced that adding
On the other hand, we lose the property that safe Rust can never leak values of uninitialized memory without invoking UB or using inline assembly or FFI. This is not a theoretical concern, this opens a whole class of security issues. (It might look a bit weird that I'm arguing against an RFC that I wrote, but my main aim was to resolve this question: either decide to add |
|
another use-case for |
|
Possible motivations, some were already mentioned:
I feel like we have seen more use cases over the years. Cc @chorman0773 @rust-lang/opsem None of these are Earth-shattering on their own but it adds up. |
|
For now it's not EB because we have no idea how we could make it EB. |
|
The easiest way to make it EB, I think, would just be to add In any case, do we have precendent on EB? It could be reasonable to say that operations that can cause EB should be unsafe as well. |
|
Why would we make it EB? It can be useful to do this, so I don't really see the benefit of allowing it if we're going to insist that you still shouldn't ever do it. |
|
Well, I don't think we have a way of stopping valgrind from trapping (though I think valgrind can trap on |
Do you have something in particular that you'd like to add to the motivation? My aim in the RFC was to present the advantages and disadvantages of
I added these two unresolved questions to the text. |
That would wholly defeat the purpose of this RFC. All programs that make use of the new functionality would be called "buggy". Remember that Miri will stop execution on EB, at least by default.
We discussed this above, didn't we? Also see the messages after that. |
|
I added a mention of the additional use cases into the RFC. I believe that the use cases that have been brought up so far are sufficient to make an informed decision whether to add |
|
I am still worried about this feature. The RFC has this Drawback:
I'd like to look at this from the perspective of the Arti project, which is a reimplementation of Tor in Rust, which we intend to replace C Tor. How should we in Tor mitigate this new risk? Obviously, Tor leaking secrets (from one connection to another, for example) is not acceptable. We could do some kind of stack-wide linting, but what then when crate authors of low-level crates such as bytemuck add We could try to manually audit these use sites (presumably all the use sites of all the freeze-tainted methods) for "bad" information leakage. But that auditing is super hard. ISTM that it's about as hard as auditing ciphers for constant-time-ness (which is almost impossible, due to similar non-guarantees to those afflicting attempts to use We could try to push back against upstream crates. But the discussions on The worst case scenario: we in Arti end up forking a few deep dependencies and trying to persuade middle-level crates to switch to our forks. That way lies madness. We might have to invent automated machinery to maintain hundreds of forks. (I don't think So, this RFC should be not merged until it contains a good answer to the question "how can a high-level Rust project avoid data leak vulnerabilities from I doubt that a good answer to this question is possible. An obvious answer is to somehow almost entirely avoid the use of Fundamentally, it is far too late in Rust's lifecycle for such a radical (and probably pervasive) weakening of the language's memory safety guarantees. |
|
Rust's promise of memory safety means that safe code doesn't have UB. Keeping secrets secret was never a part of that, and indeed it is trivial for safe code to leak secrets. Also, as mentioned above, it is already possible to write inline asm that leaks the contents of uninit memory without UB and without violating any explicit guidance ever put out by the project, and without violating the strictest proposal on the table for what such guidance may look like. #837 got rejected. That was the only time there have been team discussions about leaking of uninit memory, and the answer was to not give a no-leakage guarantee. So I don't think the claim that this new RFC weakens our memory safety guarantees holds water. If anything, this RFC clarifies our memory safety guarantees. I understand that the security community is frustrated that there is no progress on secret types and other related topics that would teach languages and compilers about crucial properties they rely on. I share that frustration. If I had the budget for it, I'd hire some people to work on this (among other things). Sadly, the companies that do have the budget for it choose to spend it on other things. But I don't think that is sufficient justification for holding back the development of language features that are entirely compatible with secret types, such as this one. |
You are contradicting yourself. Please actually update the motivation. The resulting document should be one that most people which participated in the discussion would agree to. Your goal is not to express your opinion, but the shared opinion of the majority of people assembled in this room (and the drawbacks section should be an honest attempt at representing minority opinions as well). |
To ensure everyone is on the same page: designating a behavior as Erroneous Behavior means that correct programs SHOULD NOT do the thing, but the thing does still have defined behavior if you do it anyway, and one of the possible behaviors is to halt execution with a diagnostic. The primary reason to define EB is to give "permission" for sanitizing runtimes to catch instances of EB. Integer overflow in Rust is effectively EB. Rustc will diagnose integer overflow with a panic when compiling with overflow checks, but will use the behavior of wrapping if compiling without checks. A correct Rust program SHOULD NOT rely on this behavior; it SHOULD use alternative APIs without EB when overflow is possible. This is useful as it allows developers to catch unintentional integer overflow. Making reading a frozen byte as initialized EB is saying correct programs SHOULD NOT do so in any way. For cases where the value is never used, perhaps only manipulating
So far, an argument has been that zeroing secrets is less necessary in Rust as reading dropped data is prevented by safety rules. But this isn't a whole story, as dropped data can still be read without UB. Sanitizers' ability to throw when uninit memory is used is extremely useful for testing against any potential secret leakage. But ultimately I believe it's not enough to justify not providing
By definition, EB cannot cause UB (as if it could, it would just be UB). As such, it should not be made Footnotes
|
As far as I can tell #837 was closed without answering the question. There's a difference between agreeing not to give a guarantee and not agreeing to give a guarantee. I agree that this doesn't weaken the memory safety guarantees though. Maybe one way to go forward is to say that it is EB for observable behaviour to depend on the output of |
I think EB is the wrong tool for this. EB is an outcome of the Abstract Machine. It is something that happens in a single execution -- you violate a particular precondition of a particular operation, you have EB. "Does the output of I'm all for establishing a social norm that |
|
Does EB need to qualify as observable behaviour in the abstract machine? Does C++ disallow reordering side-effecting operations with operations that might be EB? If you want a dynamic definition of non-dependence you need multiple executions, but that still leaves static semantics on the table. I suppose that wouldn't be EB anymore though. Actually checking it in the compiler would also likely require additional annotations to disambiguate bad uses of freeze from good ones, so any compile errors could only be introduced in a new edition. Does the Rust project have mechanisms for trying to establish social norms? The practice of annotating functions that would be unsound if safe with |
|
EB is whatever we define it to be. I think we should define it as an error condition the AM can end up in -- similar to UB, but a separate error condition.
Not really, no. In a language with unsafe code like Rust, dynamic semantics are the only semantics we can use for the ground truth. |
At least at a library level it isn't generally possible to definitively determine that something is not a secret. You might naively think that zeroing things like cryptographic material is sufficient, but at least for privacy it is useless to protect the keys if you're freely leaking the data encrypted by those keys.1 One resolution would be to automatically zero everything after use, but that's not ideal for performance. Instead you might get away with considering all data derived from uninitiated bytes "potentially secret", whether via the type system or documentation, and letting encapsulation deal with the rest. I strongly believe that we need a way forward where the average user does not have to engage with any of this and can credibly deny responsibility for holes in encapsulation. Both when it comes to integrity2 and privacy. Requiring people to manually zero anything that might be problematic to leak is not tenable. Footnotes
|
I don't think I'm contradicting myself. I still think that the RFC provides a good sample of use cases for
I have great respect for your work on Rust, you are the expert on the semantics and UB in the language, and in these aspect I wholly defer to your opinion. However, this RFC has the risk of introducing a major, irreversible security footgun, which could cause damage in the real world even if I understand that the RFC is not about me and should reflect the community consensus, but as the RFC author I would feel personally responsible if the RFC got accepted, As far as I can tell, you are the only one who expressed the opinion that the motivation section in the RFC is lacking an important use case, which would strongly affect the balance of pros and cons of |
|
I am very confused now. Do you want If you are not comfortable pushing for this RFC, I can take over.
Other people pointed out that the motivation is weak. And other people brought up use-cases that the motivation does not cover. Nobody else went so far as to give you direct advice for how to strengthen the RFC, but it is certainly implied in what they said. |
|
|
||
| ```rust | ||
| impl<T> MaybeUninit<T> { | ||
| const fn freeze(self) -> MaybeUninit<T>; |
There was a problem hiding this comment.
We'll definitely want to slather a pile of documentation on this function to make it clear that you should use better mechanisms for working with uninitialized memory, rather than (for instance) calling freeze and then passing an &mut T/&mut [T] to something. You could do that, but we provide better ways and you should use those.
There was a problem hiding this comment.
Maybe the documentation should enumerate a small set of high-level use cases where using this function makes sense? Here's what I can come up with:
- Whenever you have an API that mutates MaybeUninit values but doesn't have the information to determine whether they are init or not (SIMD on MaybeUninit).
- Whenever you need to temporarily eliminate padding without zeroing for use in an API that cannot handle it (atomics).
- Whenever efficiently determining the initialization status requires knowing the value if initialized (sparse sets).
| loads even for scalar types, which could otherwise be frozen as-is. This will | ||
| make it slightly harder for LLVM to optimize the code. | ||
|
|
||
| ### Freeze into `T` |
There was a problem hiding this comment.
This section explains why freeze should not return T, and I agree with this explanation. Thanks for including it.
Could the RFC include a section providing the rationale for why the freeze operation that returns MaybeUninit should be safe? I think I agree that it should be safe (@scottmcm made a good argument that it's safe for the same reason that making a raw pointer is safe: you can't actually do anything unsafe with it without unsafe code), but I'd like to see that rationale documented here.
|
The lang team discussed this today. We agreed that this is an operation we would like to add to the language. One drawback we discussed was the property of being able to keep secrets private in Rust. But that doesn't work in the presence of FFI with other languages that allow this operation. My view, which I expressed in the meeting to general agreement, is that the correct way to express that in Rust is with types that are linear ( We would welcome @RalfJung or the original author driving this RFC forward. Please re-nominate when you think it's ready for our review. @rustbot label -I-lang-nominated |
There was a problem hiding this comment.
Some copyediting, mostly around terminology relating safety and soundness:
- Safe means it can be done without an
unsafeblock. - Sound means it cannot cause UB.
A lot of this is nitpicky, feel free to take or leave it. But it's nice for an opsem RFC to be precise with opsem terminology 🙂
| In safe Rust, every data structure that uses _O(N)_ memory also necessarily has | ||
| at least _O(N)_ time complexity, because all that memory needs to be | ||
| initialized. However, there are some clever data structures, for example [this | ||
| sparse set][sparse], which can store a set of _N_ integers in _O(N)_ memory | ||
| without needing to initialize that memory. Algorithms like these assume that |
There was a problem hiding this comment.
This doesn't make sense: what does it mean for a data structure to have time complexity?
It's also not correct that all the memory belonging to any data structure must be initialized; for example, Vec::with_capacity does not need to initialize its memory.
| In safe Rust, every data structure that uses _O(N)_ memory also necessarily has | |
| at least _O(N)_ time complexity, because all that memory needs to be | |
| initialized. However, there are some clever data structures, for example [this | |
| sparse set][sparse], which can store a set of _N_ integers in _O(N)_ memory | |
| without needing to initialize that memory. Algorithms like these assume that | |
| There exist data structures designed to work with uninitialized memory. For | |
| example, [this sparse set][sparse] stores a set of up to _N_ integers in _O(N)_ | |
| memory, with a lookup algorithm that may read from uninitialized locations | |
| within that memory. Algorithms like these assume that |
| sparse set][sparse], which can store a set of _N_ integers in _O(N)_ memory | ||
| without needing to initialize that memory. Algorithms like these assume that | ||
| uninitialized memory can contain an arbitrary value, but we currently can't use | ||
| them in safe Rust, because uninitialized memory can't be read at all. |
There was a problem hiding this comment.
| them in safe Rust, because uninitialized memory can't be read at all. | |
| them in Rust, because uninitialized memory can't be read at all. |
Unsafe Rust can't read uninitialized memory either
There was a problem hiding this comment.
That's not quite true, or depends on how you define "read". Unsafe Rust can read uninit memory by using ptr::copy.
| [sparse]: https://research.swtch.com/sparse | ||
|
|
||
| However, it might already be possible to implement some of these data structures | ||
| safely by using inline assembly. For example, with the sparse set, the "story" |
There was a problem hiding this comment.
| safely by using inline assembly. For example, with the sparse set, the "story" | |
| soundly by using inline assembly. For example, with the sparse set, the "story" |
| However, this is unsafe, because `Order` contains padding bytes which are not | ||
| initialized, and it is unsafe to create a `&[u8]` that points to uninitialized | ||
| bytes. |
There was a problem hiding this comment.
unsafe -> unsound
also, this is veeery nitpick, but it's technically possible the caller passed us an &Order with initialized padding bytes, for example &*(&raw const [0; size_of::<Order>]).cast::<Order>()
| However, this is unsafe, because `Order` contains padding bytes which are not | |
| initialized, and it is unsafe to create a `&[u8]` that points to uninitialized | |
| bytes. | |
| However, this is unsound, because `Order` contains padding bytes which may be | |
| uninitialized, and it is unsound to create a `&[u8]` that points to | |
| uninitialized bytes. |
| let bytes: &[MaybeUninit<u8>] = unsafe { slice::from_raw_parts(ptr, len) }; | ||
|
|
||
| for i in 0..len { | ||
| // we need `unsafe` because of `assume_init()`; it is safe because |
There was a problem hiding this comment.
| // we need `unsafe` because of `assume_init()`; it is safe because | |
| // we need `unsafe` because of `assume_init()`; it is sound because |
| - Atomics on arbitrary small types that can contain padding | ||
| - Atomic bytewise memcpy to implement seqlocks | ||
| - A ["story"][ralfj-inline-asm] for inline assembly that performs [in-place | ||
| freeze][in-place-freeze], when the programmer can ensure that it is safe (see |
There was a problem hiding this comment.
| freeze][in-place-freeze], when the programmer can ensure that it is safe (see | |
| freeze][in-place-freeze], when the programmer can ensure that it is sound (see |
| bit pattern is a valid value of these types. | ||
|
|
||
| Multiple calls of the `freeze()` function on the same value may return different | ||
| values of the uninitialized bytes, the argument to this function is not modified |
There was a problem hiding this comment.
comma splice
| values of the uninitialized bytes, the argument to this function is not modified | |
| values of the uninitialized bytes; the argument to this function is not modified |
|
|
||
| ### Can we just use a volatile read? | ||
|
|
||
| The compiler must translate volatile reads into actual memory loads, it cannot |
There was a problem hiding this comment.
| The compiler must translate volatile reads into actual memory loads, it cannot | |
| The compiler must translate volatile reads into actual memory loads. It cannot |
comma splice
| (like reading from a memory-mapped hardware device). | ||
|
|
||
| However, volatile reads that are inside a Rust allocation (memory accessible to | ||
| a Rust program), still need to [obey the same rules][read-volatile] as normal |
There was a problem hiding this comment.
| a Rust program), still need to [obey the same rules][read-volatile] as normal | |
| a Rust program) still need to [obey the same rules][read-volatile] as normal |
| into the `freeze` instruction in LLVM, at least for scalar types, so that code | ||
| that interacts with C bitfields can use `freeze()` to preserve values of | ||
| initialized bits in partly uninitialized bytes. However, the documentation | ||
| should make it clear that this is not a stability promise, Rust currently does |
There was a problem hiding this comment.
| should make it clear that this is not a stability promise, Rust currently does | |
| should make it clear that this is not a stability promise. Rust currently does |
| taking the input by value: | ||
|
|
||
| ```rust | ||
| fn freeze<T>(input: &MaybeUninit<T>) -> MaybeUninit<T>; |
There was a problem hiding this comment.
I fear that the current way of making "by-value freeze" and not "copying freeze" the underlying intrinsic would lead to worse code generation when one attempts to write a "copying freeze." Presumably, "copying freeze" is the common case. It is what is wanted in all examples in the motivation section in this document, since they always start with a reference/pointer to the data (instead of moving/copying it into the function).
Consider the ways of implementing this "freeze-copy" operation. It would look something like this:
fn freeze<T>(input: &MaybeUninit<T>) -> MaybeUninit<T> {
let input_copy_1 = unsafe { ptr::read(input as *const MaybeUninit<T>) }; // we do not get to assume `T: Copy` in general.
let input_copy_2 = input_copy_1.freeze();
input_copy_2
}Presumably, the emitted MIR would have the local twice, which means the stack frame is twice as large as it needs to be. This is suboptimal, but enforced by the primitive which takes a value, not a pointer to it. As a consequence of this, the function also copies the data around twice, doubling the runtime (this is maybe the more real concern). Perhaps LLVM/MIR optimizations are able to reduce the stack frame size again, but experience suggests that Rust is not very good at this kind of optimization. Once this function is implemented, we should take care that this does not take more stack space/runtime than necessary. Perhaps the only way to guarantee this, is to have this "copying" function as the primitive and implement the other operations based on it.
View all comments
Introduce an operation similar to the LLVM
freezeinstruction, which converts uninitialized values into initialized but arbitrary values:The biggest disadvantage of adding the
freezeoperation is that a Rust program can leak a secret that was previously stored in the uninitialized memory without triggering UB.Important
Since RFCs involve many conversations at once that can be difficult to follow, please use review comment threads on the text changes instead of direct comments on the RFC.
If you don't have a particular section of the RFC to comment on, you can click on the "Comment on this file" button on the top-right corner of the diff, to the right of the "Viewed" checkbox. This will create a separate thread even if others have commented on the file too.
Rendered