Check if TCS is a null pointer on SGX

[joboet]

The EENTER instruction only checks if the TCS is aligned, not if it zero. Saying the address returned is a NonNull<u8> (for which Tcs is a type alias) is unsound. As well-behaved runners will not put the TCS at address zero, so the definition of Tcs is correct. However, std should check the address before casting it to a NonNull.

ping @jethrogb @raoulstrackx
@rustbot label I-unsound

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[jethrogb]

I don't think this check is necessary. You mention well-defined runners, but that doesn't really matter. The question is about well-defined enclaves, and those will never have a TCS at 0.

[thomcc]

I don't know whether or not it's needed, but this changes us from assuming an external C function returns a NonNull to checking at runtime and panicking if it was null. That seems worth doing to me (this check should be cheap, correctly predicted, and we do similar things on other targets anyway), even if it's not believed to be possible.

Whether or not it's possible might matter for prioritization (since it's not actually a soundness fix if it was impossible), but I'm in favor either way.

@bors r+

[bors]

📌 Commit 4f157dac10506393e806c25eb951f83a381598a4 has been approved by thomcc

It is now in the queue for this repository.

[jethrogb]

@thomcc have you verified it's safe to panic in this context? If not, please r-.

In general, perhaps defer to platform maintainers on platform-specific code.

[thomcc]

Fair enough.

@bors r-

[thomcc]

Actually, the panic can only happen in the case we returned a NonNull that was actually null, which would be UB as well, so I think a panic is very easily justified.

[thomcc]

AFAICT it should be fine to panic in the places where this is called, and if the call truly cannot return null then it doesn't matter if we panic.

Note that we'd prefer to panic over UB even if it's only possible due to a buggy or incorrectly configured system -- on other targets this sort of caution this has caught issues that show up in emulation, for example.

[joboet]

Ok, I've now gone through a bit of the SGX tooling. IIUC (this is just me taking notes), each enclave is fitted with a fixed number of TCS structures upon conversion of the output ELF to SGXS (sort of like ELF, but for SGX), which are never placed at address zero in the enclave. This layout is part of the enclave signature, so misbehaving runners that try to change this will change the enclave signature and will thus be caught.

Therefore, this is not unsound (sorry for jumping to wrong conclusions here). Still, as @thomcc mentioned, the check is cheap and perhaps this will catch some issues in the future...

@rustbot label -I-unsound -I-prioritise [ahem!]

[rustbot]

Error: Parsing relabel command in comment failed: ...'tise [ahem' | error: a label delta at >| '!]'...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[jethrogb]

I think it may be ok to unwind, but it's being called next to code that definitely shouldn't unwind. Perhaps use rtunwrap to be safe. Also reword the error message.

[thomcc]

@rustbot author

[joboet]

@rustbot label +S-waiting-on-review -S-waiting-on-author
I hope the wording is better this time?

[rustbot]

Error: The "Ready" shortcut only works on pull requests.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[thomcc]

Seems good to me.

@bors r+

[bors]

📌 Commit 2fa5808 has been approved by thomcc

It is now in the queue for this repository.

[bors]

⌛ Testing commit 2fa5808 with merge 98e1f04...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing 98e1f04 to master...

[rust-timer]

Finished benchmarking commit (98e1f04): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	3.3%	[3.3%, 3.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.0%	[-5.4%, -2.6%]	2
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	2
All ❌✅ (primary)	-1.5%	[-5.4%, 3.3%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	3.1%	[1.8%, 5.7%]	9
Regressions ❌ (secondary)	3.0%	[2.3%, 3.5%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[1.8%, 5.7%]	9

Footnotes

1. the arithmetic mean of the percent change ↩ ↩² ↩³

2. number of relevant changes ↩ ↩² ↩³

[jethrogb]

This is fine, but not sure why you used rtabort instead of rtunwrap as requested.

[joboet]

This is fine, but not sure why you used rtabort instead of rtunwrap as requested.

Because rtabort allows setting custom abort messages instead of just printing the generic "unwrap failed".