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

Fix incorrect inline assembly usage in s2n_rand_rdrand_impl #4310

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

DimasKovas
Copy link
Contributor

Description of changes:

This PR fixes an incorrect inline assembly usage in s2n_rand_rdrand_impl.

The compiler may reuse a register for both input and output of an inline assembly statement, and the input should be consumed before writing to any output registers.

In s2n_rand_rdrand_impl there is an implicit input (an address of success variable) that is read by setc instruction after rdrand had already written to an output register. If the compiler reuses a register for input and output in that case, the setc instruction will try to write to an incorrect memory address, causing SIGSEGV or memory corruption.

Marking the output register as earlyclobber (&) prevents the compiler from reusing the output register for input.

We've encountered such SIGSEGV in s2n_rand_rdrand_impl during running our application under ASAN build, which actually produced the binary code described above.

Here is the related issue with the explanation of the bug:
google/sanitizers#1629

And here is a more detailed explanation from gcc documentaion on how to use inline assembly:

Use the & constraint modifier (see Constraint Modifier Characters) on all output operands that must not overlap an input. Otherwise, GCC may allocate the output operand in the same register as an unrelated input operand, on the assumption that the assembler code consumes its inputs before producing outputs. This assumption may be false if the assembler code actually consists of more than one instruction.

The same problem can occur if one output parameter (a) allows a register constraint and another output parameter (b) allows a memory constraint. The code generated by GCC to access the memory address in b can contain registers which might be shared by a, and GCC considers those registers to be inputs to the asm. As above, GCC assumes that such input registers are consumed before any outputs are written. This assumption may result in incorrect behavior if the asm statement writes to a before using b. Combining the & modifier with the register constraint on a ensures that modifying a does not affect the address referenced by b. Otherwise, the location of b is undefined if a is modified before using b.

Testing:

I've build our application with s2n-tls under ASAN and checked that the SIGSEGV has gone away.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@DimasKovas DimasKovas mentioned this pull request Dec 4, 2023
@DimasKovas DimasKovas changed the title Mark inline asm output as earlyclobber Fix incorrect inline assembly usage in s2n_rand_rdrand_impl Dec 4, 2023
Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! This looks reasonable to me, but I'd appreciate a second opinion from @torben-hansen

Copy link
Contributor

@torben-hansen torben-hansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. One can clearly see clobbering from the godbolt, linked here.

Without the early-clobber constraint: https://godbolt.org/z/e13W6zjan
with early-clobber constraint: https://godbolt.org/z/Mdx8zMhsa

Copy link
Contributor

@goatgoose goatgoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@goatgoose goatgoose enabled auto-merge (squash) December 6, 2023 20:10
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

Successfully merging this pull request may close these issues.

4 participants