range-based constant-flow base64 #5108
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Base64 decoding uses equality comparison tests for characters that don't leak information about the content of the data other than its length, such as whitespace. Do this with '=' as well, since it only reveals information about the length. This way the table lookup can focus on character validity and decoding value. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Instead of doing constant-flow table lookup, which requires 128 memory loads for each lookup into a 128-entry table, do a range-based calculation, which requires more CPU instructions per range but there are only 5 ranges. Experimentally, this is ~12x faster on my PC (based on programs/x509/load_roots). The code is slightly smaller, too. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Document what each local variable does when it isn't obvious from the name. Don't reuse a variable for different purposes. This commit has very little impact on the generated code (same code size on a sample Thumb build), although it does fix a theoretical bug that 2^32 spaces inside a line would be ignored instead of treated as an error. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Instead of doing constant-flow table lookup, which requires 64 memory loads for each lookup into a 64-entry table, do a range-based calculation, which requires more CPU instructions per range but there are only 5 ranges. I expect a significant performance gain (although smaller than for decoding since the encoding table is half the size), but I haven't measured. Code size is slightly smaller. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
n was used for two different purposes. Give it a different name the second time. This does not seem to change the generated code when compiling with optimization for size or performance. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
To test c <= high, instead of testing the sign of (high + 1) - c, negate the sign of high - c (as we're doing for c - low). This is a little easier to read and shaves 2 instructions off the arm thumb build with arm-none-eabi-gcc 7.3.1. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
I had originally thought to support directories with mbedtls_x509_crt_parse_path but it would have complicated the code more than I cared for. Remove a remnant of the original project in the documentation. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Add unit tests for mask_of_range(), enc_char() and dec_value(). When constant-flow testing is enabled, verify that these functions are constant-flow. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
This is part of the definition of the encoding, not a choice of test parameter, so keep it with the test code. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
digits is also a local variable in host_test.function, leading to compilers complaining about that shadowing the global variable in test_suite_base64.function. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
gilles-peskine-arm
added
bug
needs-review
mpg
approved these changes
Oct 27, 2021
|
IMO the forward-port was straightforward enough that this doesn't need a second reviewer. |
mpg
added
single-reviewer
mpg
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #4814: in Mbed TLS 2.26.0 and 2.16.10, we made the base64 code constant-flow by changing table lookup into constant-time table lookup (look up every item and or them together). This had a significant performance cost. This pull request uses a different approach: instead of doing a constant-flow table lookup, do a range-based or. Since base64 has only 5 ranges, as opposed to 64/128 table entries (encoding/decoding), this turns out to be a significant performance improvement. There is also a slight gain in code size (but still a loss compared to the original non-constant-time table approach). See #4819 for benchmarks.
This is a mostly straightforward rebase of the 2.2x version. A single reviewer may be enough, I'll leave that at the discretion of the first reviewer. Rebase notes:
"mbedtls/build_info.h"insted ofMBEDTLS_CONFIG_FILE.programs/Makefile: theAPPSlist is no longer defined with extensions.Backports: 2.2x, 2.16.