SGX `UnsafeList` is unsound, `WaitQueue` can execute UB

[robofinch]

I was looking at this SGX code: https://github.com/rust-lang/rust/blob/0312931d8c0ba1a28268a12c06202b68cbc65f76/library/std/src/sys/pal/sgx/waitqueue/unsafe_list.rs

It's a linked-list implementation that freely mixes pointers and references, which is a recipe for disastrous UB. Indeed, simply pushing and popping an element executes UB, as detected by Miri even with Tree Borrows.

Here's a playground based on the SGX code which executes UB: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=2655d6852f717956f2555a6688dd86c2

Since remove takes an exclusive/mutable reference, the fallback path of WaitQueue::wait_timeout can execute UB as far as I can tell:

rust/library/std/src/sys/pal/sgx/waitqueue/mod.rs

Line 180 in 0312931

guard.queue.inner.remove(&mut entry);

(To be precise, creating the new mutable reference and passing it into a function invalidates any pointers to entry already in the list. If entry was the first entry, that includes self.head_tail. Then, assert!(!self.is_empty()) executes UB. Maybe I need to add an "unless T: !Unpin" carve-out, not sure.)

Solution

Two things: avoid taking exclusive/mutable references as inputs to the UnsafeList, and exclusively use raw pointers in the linked list.

SGX's waitqueue code pushes internally mutable types into the list, so taking mutable references to entries should be unnecessary. Shared references should work fine.

Instead of creating mutable references for the sake of doing writes... just write using the raw pointers. Same for reads, don't create temporary references. Reference->pointer conversion should happen as soon as possible after taking an input, and pointer->reference conversion should happen as late possible just before returning an output.

[RalfJung]

Cc @tvsfx @jethrogb

[jethrogb]

@theemathas this is not requires-nightly, this code is exercised in std's main synchronization primitives when compiling for the SGX target.

[joboet]

I think this is a duplicate of #114581?

Anyway, I plan to fix this by removing all that platform-specific stuff in favour of a shared queue-based implementation (#127578). Unfortunately this requires a few other things too, like NetBSD support in miri and a fix for #121950, and I haven't gotten around to that yet.

[jethrogb]

It is a duplicate, in a way. The discussion in that other issue concludes that whether this is allowed is still undecided. This issue claims it's plainly unsound.

In any case, I have a PR about to go that switches to raw pointers while keeping the “public” UnsafeList API the same.

[RalfJung]

I think this is a duplicate of #114581?

I don't think so... that issue is very hard to reproduce UB that arises in specific ABA situations. According to the issue description, the SGX queue has UB even for trivial push+pop.

[jethrogb]

@joboet How close are you to replacing the queue? This issue inspired me to look at using the “new” pinning feature in this code, but if that's going to be moot very soon I'll not waste my time.

[joboet]

That will probably take a few months at minimum, so don't wait for me...