Remove integer arithmetic on values converted from pointers

[briansmith]

We have done some work already to remove some uses of uintptr_t, such as bccbeb2. We should remove all of them. If, for some reason, we need to keep any, then we should encapsulate those uses in functions as described in http://trust-in-soft.com/when-in-doubt-express-intent-and-leave-the-rest-to-the-compiler/.

The C standard says that it must be possible to convert a pointer to a uintptr_t and then convert the uintptr_t back to a pointer, losslessly. However, the values of the resultant integers are implementation-defined. Also, uintptr_t is optional in C11 and it is not guaranteed that any integer type can hold the value of a pointer.

"'6.2.8 Alignment of objects' in C11 hints that it should work in a natural way" and the mapping is "'intended to be consistent with the addressing structure of the exec. env.'" as pointed out by @AlekseyCherepanov on Twitter. However, I don't think we can rely on these, especially when it comes to undefined behavior prevention and compiler bug avoidance.

Note that some pointer -> integer conversions are not using uintptr_t or intptr_t. It is common for the code to use size_t and I've also seen casts to int. Those need to be found and fixed.

[briansmith]

Ideally, we would use C's alignas more, and require more inputs to be aligned. However, #36 is in the way; we don't have a way of aligning values in Rust other than doing the same uintptr_t arithmetic that is done in C, in Rust, where such things are even less well-specified.

[AlekseyCherepanov]

Non-technical comment: you refer to @ch3root on Twitter, he is @ch3root on github too. My name is Aleksey, his name is Alexander. Thanks!

[ch3root]

Yep, I'm @ch3root on github too. I filled the name in the profile now:-)

Quick search with coccinelle gives the following cases of casts of pointers to non-pointers:

ring/crypto/bn/montgomery.c

Line 338 in 72db3b2

nrp = (BN_ULONG *)(((uintptr_t)rp & ~m) | ((uintptr_t)ap & m));

ring/crypto/poly1305/poly1305_vec.c

Line 85 in 72db3b2

return (poly1305_state_internal *)(((uint64_t)state + 63) & ~63);

ring/crypto/modes/gcm.c

Line 581 in 72db3b2

if (STRICT_ALIGNMENT && ((uintptr_t)in | (uintptr_t)out) % sizeof(size_t) != 0) {

ring/crypto/modes/gcm.c

Line 744 in 72db3b2

if (STRICT_ALIGNMENT && ((uintptr_t)in | (uintptr_t)out) % sizeof(size_t) != 0) {

ring/crypto/cipher/internal.h

Line 145 in 72db3b2

assert(((uintptr_t)ctx_buf) % ctx_struct_alignment == 0);

ring/crypto/cipher/internal.h

Line 159 in 72db3b2

assert(((uintptr_t)ctx_buf) % ctx_struct_alignment == 0);

[briansmith]

on-technical comment: you refer to @ch3root on Twitter, he is @ch3root on github too. My name is Aleksey, his name is Alexander. Thanks!

Thank you and sorry!

[briansmith]

Yep, I'm @ch3root on github too. I filled the name in the profile now:-)

Thanks and sorry!

Quick search with coccinelle gives the following cases of casts of pointers to non-pointers:

There's also, at least, this one in

ring/crypto/poly1305/poly1305_arm.c

Line 186 in 72db3b2

fe1305x2 *const r = (fe1305x2 *)(st->data + (15 & (-(int)st->data)));

:

  fe1305x2 *const r = (fe1305x2 *)(st->data + (15 & (-(int)st->data)));

Thanks for the tip about coccinelle.

[ch3root]

Yeah, coccinelle doesn't find everything -- sometimes it doesn't see enough info about types etc. But in this case it's not its fault -- I was searching pointers only and arrays were skipped. A bit more advanced script additionally finds three instances of the code you quoted -- the one you linked to and these two:

ring/crypto/poly1305/poly1305_arm.c

Line 212 in 72db3b2

fe1305x2 *const r = (fe1305x2 *)(st->data + (15 & (-(int)st->data)));

ring/crypto/poly1305/poly1305_arm.c

Line 260 in 72db3b2

fe1305x2 *const r = (fe1305x2 *)(st->data + (15 & (-(int)st->data)));

The script is attached -- pointer-cast.cocci.zip. Run it as spatch --sp-file pointer-cast.cocci --include-headers . .

BTW the code you quoted could invoke UB when there is signed integer overflow in unary minus.

[briansmith]

Also MOD_EXP_CTIME_ALIGN and related things in crypto/bn/exponentiation.c.

[briansmith]

I notified OpenSSL of the issues in BN_from_montgomery_word in https://www.mail-archive.com/[email protected]/msg44759.html. Note that any use of BN_from_montgomery_word should be considered a bug in ring that will be fixed.

[briansmith]

Also MOD_EXP_CTIME_ALIGN and related things in crypto/bn/exponentiation.c.

I submitted a PR to BoringSSL: https://boringssl-review.googlesource.com/#/c/7492/. But, I think they don't want to take it.

[briansmith]

poly1305/poly1305_vec.c#L85
modes/gcm.c#L581
modes/gcm.c#L744

Fixed.

ring/crypto/cipher/internal.h

Line 145 in 72db3b2

assert(((uintptr_t)ctx_buf) % ctx_struct_alignment == 0);

ring/crypto/cipher/internal.h

Line 159 in 72db3b2

assert(((uintptr_t)ctx_buf) % ctx_struct_alignment == 0);

These are just assertions verifying that alignment is done correctly when traversing the Rust <-> C interface. We may be able to remove this when we replace crypto/modes/gcm.c with Rust code.

[briansmith]

A while back @ch3root wrote:

Quick search with coccinelle gives the following cases of casts of pointers to non-pointers:

@ch3root, could you share how you used coccinelle to do the searches above? Any chance you could share your scripts?

[briansmith]

As of now, there's no use of uintptr_t or intptr_t in ring's C codebase any more. Further, there are no more pointer-to-size_t casts either. So, I believe this is fixed! I'd love to use coccinelle to check this too, though.

[briansmith]

Done (in the C code)