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. |
RalfJung
added
the
I-lang-nominated
|
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. |
traviscross
added
the
T-lang
if you want a slower seqlock that works on arbitrary types including padding, but not including anything containing pointers, you can use you'll want atomic bytewise memcpy for full speed seqlocks, since the compiler can do much larger loads and stores with that. |
|
Ah right, you can do "atomic bytewise memcpy at home". There's also code somewhere that uses a |
|
Cc @thomcc as another freeze supporter, maybe you can help gather more usecases. :) |
|
My two cents: to my understanding, this RFC would allow soundly implementing a safe function along the lines of If that's the case, this undermines what I consider to be a big part of the "memory safety" promise, which I think is extremely undesirable. If it's not the case, then sorry for my misunderstanding :-) If I did not misunderstand, I would be much more comfortable if For the case with atomics, is there a reason I am missing that would make a wrapper that copies the type into a zeroed u64 (or similar) before atomic operations not work? |
I don't think so. |
Something like this: fn read_some_arbitrary_process_memory(len: usize) -> &'static [u8] {
assert_ne!(len, 0);
let layout = Layout::array::<u8>(len).unwrap();
// SAFETY: `layout` is not 0-sized.
let uninit: *mut MaybeUninit<u8> = unsafe {std::alloc::alloc(layout)}.cast();
let ret = Box::leak(vec![0u8; len].into_boxed_slice());
for i in 0..len {
// SAFETY: i <= len and we own the memory
let val = unsafe { uninit.add(i).read() };
let val = freeze(val);
// SAFETY: we just `freeze`'d the MaybeUninit, and every u8 bit pattern is valid
ret[i] = unsafe { val.assume_init() };
}
ret
}If there's UB here under the proposed semantics of |
|
Oh, I don't think declaring it UB is a meaningful defense, and we have years of accumulated requests to allow freezing, so the benefits outweigh the cost. Also note that if you replace People have different ideas about what "memory safety" means. To me it means "no UB". Including non-interference properties such as the one you are alluding to would move the goalpost by miles. The entire stack from LLVM to rustc is not equipped to deal with any kind of reasoning about "not leaking secrets". This is not a new observation, and not a new problem introduced by this RFC. It is a problem, and we know the basic tools needed to tackle it, but none of the players in the ecosystem that have the resources to turn those tools into reality seem sufficiently interested in making progress here so nothing happens. That should not stall progress for other useful features such as freeze, IMO. |
|
Well, if that'd be sound to do with inline asm, then so be it :-) (I was under the impression that inline asm was also disallowed from reading uninit memory, but I can see how that would be hard to formulate) |
|
The inline asm situation is complicated -- but tl;dr: if you have an inline asm blob that reads any mapped memory (initialized or not), and you specify it as "just returns arbitrary data", that pretty much has to be okay. People use inline asm to read all sorts of stuff that's not memory in the AM, like stack pointers, or to read memory that they don't have the right to read as per the aliasing rules, and that's all fine as long as they don't make any assumptions about the bytes they see. Given that we want to allow many of these patterns, there's not really any way I know that we could say "oh but specifically for uninit memory this is not allowed". Crucially, such inline asm is different from freeze in that as far as reasoning about the code goes, it always returns arbitrary data, even if the memory happens to be initialized. But in practice it can still return the actual data that sits there, so if you are thinking in terms of leaking secrets, that makes no difference. |
|
I understand that the formal models Rust currently uses cannot even formulate guarantees about nondeterministic code not returning your cryptographic keys, but in practice some sources of nondeterminism are less likely to do that than others. I think that's worth considering. (I agree that asm blocks can be just as bad as freeze here, but asm blocks are scary) The number of requests for I think that |
Basically none, as far as I can recall. That's why it is not in my list. |
|
@RalfJung Just a question about the procedure, what are going to be next steps for the RFC? Is there still anything that needs to be resolved (other than the question of whether we want this operation at all), or anything else that I can do to move this forward? |
|
My understanding of the current status is that you got a bunch of feedback that should be incorporated into the text, especially for the motivation section. I haven't followed to what extent you stayed on top of the various subthreads here in terms of updating the RFC to resolve their concerns. |
|
Here are all the unresolved threads and conversations:
Do you think that some of the points above should be incorporated into the text? |
|
The motivation section definitely needs to be updated and extended. Even you yourself didn't seem very convinced by the original motivation any more for a while. ;)
Then please put this under "unresolved questions".
You could add an unresolved question along the lines of
|
|
Should this be an unsafe operation? Also, should this require a separate |
|
@joshtriplett This is available on any type, not just bytes. So if this |
|
As proposed, with signature |
|
The It doesn't need to be unsafe because it doesn't cause undefined behaviour at all. There are security implications, but we don't consider |
|
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. |
This branch has not been deployed
18 participants
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