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.

Sign up for GitHub

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

Specify previously missed register clobbers in AES-NI asm blocks #9809

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
+9 −4
Open

Specify previously missed register clobbers in AES-NI asm blocks #9809

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0abc1ce
Specify register clobbers in mbedtls_aesni_crypt_ecb()
solardiz Nov 30, 2024
4393dff
Specify previously missed XMM register clobbers in AES-NI asm blocks
solardiz Dec 8, 2024
6b2ca18
Add change log entry on AES-NI asm block fixes
solardiz Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tf-psa-crypto/drivers/builtin/src/aesni.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ int mbedtls_aesni_crypt_ecb(mbedtls_aes_context *ctx,
"movdqu %%xmm0, (%4) \n\t" // export output
:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a changelog entry file. Even if we don't know for sure that a platform is affected, insufficient clobbers are a bug. If the next GCC/Clang/MSVC/… triggers the bug, users should be informed of which version of Mbed TLS fixed it.

Copy link
Author

Choose a reason for hiding this comment

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

Would you like me to include this under Security, Bugfix, or Changes? I notice that a previous related change was somehow under Changes:

Changes
[...]
   * Fix clobber list in MIPS assembly for large integer multiplication.
     Previously, this could lead to functionally incorrect assembly being
     produced by some optimizing compilers, showing up as failures in
     e.g. RSA or ECC signature operations. Reported in #1722, fix suggested
     by Aurelien Jarno and submitted by Jeffrey Martin.

Copy link
Contributor

Choose a reason for hiding this comment

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

Under Bugfix, please.

Looking at the history, it seems we messed up the changelog sections in the 2.17.0 release. Originally that entry was under Bugfix.

: "r" (ctx->nr), "r" (ctx->buf + ctx->rk_offset), "r" (mode), "r" (input), "r" (output)
: "memory", "cc", "xmm0", "xmm1");
: "memory", "cc", "xmm0", "xmm1", "0", "1");

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm checking the other asm blocks in this file.

In aesni_setkey_enc_128, I think xmm0 and xmm1 are missing from the clobber list, right? And in aesni_setkey_enc_192 and aesni_setkey_enc_256, same and also xmm2. With your fix, the rest look fine to me.

Copy link
Author

Choose a reason for hiding this comment

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

In aesni_setkey_enc_128, I think xmm0 and xmm1 are missing from the clobber list, right? And in aesni_setkey_enc_192 and aesni_setkey_enc_256, same and also xmm2. With your fix, the rest look fine to me.

Oh, you're right. Is this something you'd fix separately from this PR?

OTOH, I think in mbedtls_aesni_gcm_mult, the clobber list unnecessarily includes cc - I think none of those instructions modify flags. But this may be better to leave as-is at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd appreciate it if you could fix those clobber lists while you're at it, so we can say we fixed the assembly in the AESNI code and not just in one function. But if not we'll make a follow-up pull request.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I intend to fix these as well (and credit you in the commit message for noticing them), but it's a busy week and it's taking me a while to get back to "free time" work again. Just letting you know that I accepted the task, but can't handle it as a high priority.

Copy link
Author

Choose a reason for hiding this comment

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

I've just added a commit fixing the missed XMM clobbers and adding a changelog entry. I hope this is as desired.

I tested these same changes in a copy/revision of the code as we integrated it in JtR. I temporarily reverted from usage of intrinsics to asm, rebuilt JtR, and ran our tests - so whatever functions we do use there were tested. I also checked the aesni.o text section size, which remained unchanged at 1445 bytes for me (which does not guarantee no increase in actually used code as the function entry points are aligned, so each previous function is padded).

I never tried building/testing mbedTLS proper (sorry!) and quickly trying to do so now first gave me this:

$ make check
Makefile:19: *** /framework/exported.make not found (and does not appear to be a git checkout). Please ensure you have downloaded the right archive from the release page on GitHub..  Stop.

which made me look inside the Makefile, see the other error message nearby (is the condition on which message to print maybe wrong?) and then do:

$ git submodule update --init
Submodule 'framework' (https://github.com/Mbed-TLS/mbedtls-framework) registered for path 'framework'
Cloning into '/home/user/mbedtls/framework'...
Submodule path 'framework': checked out 'df0144c4a3c0fc9beea606afde07cf8708233675'

Then rerunning make check fails at:

  CC    src/test_helpers/ssl_helpers.c
make[1]: Leaving directory '/home/user/mbedtls/tests'
make[1]: Entering directory '/home/user/mbedtls/library'
  Gen   ../tf-psa-crypto/core/psa_crypto_driver_wrappers.h ../tf-psa-crypto/core/psa_crypto_driver_wrappers_no_static.c
Traceback (most recent call last):
  File "/home/user/mbedtls/library/../scripts/generate_driver_wrappers.py", line 18, in <module>
    import jsonschema
ModuleNotFoundError: No module named 'jsonschema'

I'm not eager to install a Python module without creating a dedicated environment for this testing first, so I stopped here.

I mention this as maybe-useful feedback on maybe improving error messages and maybe relaxing build/test dependencies for new users.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I intend to fix these as well (and credit you in the commit message for noticing them), but it's a busy week and it's taking me a while to get back to "free time" work again. Just letting you know that I accepted the task, but can't handle it as a high priority.

No worries, I know all about “free time”! If you prefer, I can take over and finish the patch. Well, you've gone ahead and updated, thank you very much, but I can take over if there further updates are needed.

I never tried building/testing mbedTLS proper (sorry!) and quickly trying to do so now first gave me this: (…)

Feedback noted. Unfortunately, while we'd like to get rid of the Python dependencies, that would require significant engineering work. For what it's worth, releases should be fine on both counts, you just run into these difficulties when you download from a development branch. Hey, at least you didn't have to run autotools!


return 0;
Expand Down