Fix misuse of signed ints in the HAVEGE module #149
Conversation
The elements of the HAVEGE state are manipulated with bitwise operations, with the expectations that the elements are 32-bit unsigned integers (or larger). But they are declared as int, and so the code has undefined behavior. Clang with Asan correctly points out some shifts that reach the sign bit. Since these are supposed to be 32-bit unsigned integers, declare them as uint32_t. This is technically an API break, since the type mbedtls_havege_state is exposed in a public header. However normal applications should not be affected.
gilles-peskine-arm
added
bug
library/havege.c
Outdated
| int PT1, PT2, *WALK, RES[16]; | ||
| int PTX, PTY, CLK, PTEST, IN; | ||
| size_t n = 0; | ||
| unsigned i; |
There was a problem hiding this comment.
Why should i not be size_t? It's used to index RES, as in ONE_ITERATION (RES[(i - 8) ^ PTY]), and array indices tend to be size_t.
There was a problem hiding this comment.
i only takes values from 0 to 15 and is used in an inner loop, so it's more correct to have it of “the most efficient type” than “the correct type for indexing arrays”. But it isn't worth giving the reader pause for that. The compiler can easily recognize what the actual range of i is and optimize accordingly anyway.
There was a problem hiding this comment.
We don't officially require C99 (well, I think we made a blog post that says we do, but we still test with non-C99 compilers). We use stdint.h but we only assume that it provides the “basic” types: uintNN_t. Are we ok with adding a requirement for uint_ADJECTIVENN_t? Should we start using it in more places?
| @@ -38,6 +38,7 @@ | |||
| #include "mbedtls/timing.h" | |||
| #include "mbedtls/platform_util.h" | |||
|
|
|||
| #include <stdint.h> | |||
There was a problem hiding this comment.
Style: Should we bother to include stdint.h again as we'd have already included it via including havege.h?
There was a problem hiding this comment.
I tend to prefer to include headers for what they define, not for what headers they pull in. According to this style, since havege.c uses uint32_t, it includes stdint.h.
There was a problem hiding this comment.
Is this the convention we use in the rest of Mbed TLS as well? I'm happy to use whatever the library already does.
It's slightly more efficient to not #include a header if you know the types are already available, as they must since the declarations we use from havege.h require it, because the preprocessor doesn't need to read in that file at all. For large projects, and especially on slow filesystems (e.g. Windows, network drives), this can add up to a big savings in compile time. It's not uncommon to have a style that encourages efficiency. (C++ style guides recommending pre-increment operators over post-increment operators in for loops is one example of this.)
There was a problem hiding this comment.
I don't remember that we ever discussed this, and I don't think we're doing or not-doing this with any noticeable consistency.
All our header files have garden-variety include guards. Compilers typically optimize well for that case. In any case, compilation time is the least of our worries. This is not a 100000-file C++ project. Being a security project, we value security, readability and ease of maintenance rather more than runtime performance, let alone build-time performance. Also, if you want to optimize compilation time, the biggest thing by far would be to get rid of cmake!
|
@k-stachowiak CI fails on Mbed TLS builds. There's an incompatible change in a header file, so the CI won't pass if it runs with one side updated but not the other. The CI passes on Mbed-TLS/mbedtls#2699 which has the header changes on both sides. |
Patater
removed
the
needs: review
|
Note that #149 and Mbed-TLS/mbedtls#2699 need to be merged in the same time frame, because they make an incompatible change to a header file. The sequence of operations is as follows:
|
The elements of the HAVEGE state are manipulated with bitwise operations, with the expectations that the elements are 32-bit unsigned integers (or larger). But they are declared as
int, and so the code has undefined behavior. Clang with Asan correctly points out some shifts that reach the sign bit.Since these are supposed to be 32-bit unsigned integers, declare them as
uint32_t.This is technically an API break, since the type
mbedtls_havege_stateis exposed in a public header. However normal applications should not be affected.Fix Mbed-TLS/mbedtls#2598
PR extracted from #144 which combined this with other things needed to pass Clang+Asan with all configuration options.
When this PR is merged, it'll break the build in mbedtls due to the incompatible change in
havege.h(this is what I mean by the label "needs: preceding PR"). Mbed-TLS/mbedtls#2699 will be needed to repair the build, so the merge of these two PRs should be coordinated.