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 aarch64 assembly for bignum multiplication #4968

Conversation

davidhorstmann-arm
Copy link
Contributor

@davidhorstmann-arm davidhorstmann-arm commented Sep 22, 2021

Add missing memory constraint in aarch64 bignum multiplication code to remedy Clang 12+ issues. Fixes #4962.
Part of the larger super-issue #4943.

Tested with the following steps:

mkdir build && cd build
CC=clang CFLAGS="--target=aarch64-linux-gnu" cmake -DCMAKE_BUILD_TYPE=Release ..
make -j8
cd tests
qemu-aarch64 -L /usr/aarch64-linux-gnu ./test_suite_mpi

Status

READY

Requires Backporting

Yes
Only to 2.x as 2.16 does not have this aarch64 assembly

Add memory constraints to the aarch64 inline assembly in MULADDC_STOP.
This fixes an issue where Clang 12 and 13 were generating
non-functional code on aarch64 platforms. See Mbed-TLS#4962, Mbed-TLS#4943
for further details.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@davidhorstmann-arm davidhorstmann-arm added bug component-crypto Crypto primitives and low-level interfaces needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Sep 22, 2021
xffbai
xffbai previously approved these changes Sep 24, 2021
Copy link
Contributor

@xffbai xffbai left a comment

Choose a reason for hiding this comment

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

LGTM and tested good on local arm machine.

@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Sep 24, 2021
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

LGTM except I think we should combine the changelog entries.

I reviewed the code change and I verified that this fixes the bug with Clang 12 on Ubuntu 20.04, cross-compiling for aarch64 and testing with Qemu.

@@ -0,0 +1,4 @@
Bugfix
* Add missing memory constraints in aarch64 inline assembly for
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should combine the two changelog entries that concern the same bug on different architectures.

@gilles-peskine-arm gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Sep 24, 2021
Combine the changelog entries for the memory constraints fix on
aarch64 and amd64, since these are essentially fixing the same
issue.

Signed-off-by: David Horstmann <david.horstmann@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-backports Backports are missing or are pending review and approval. labels Sep 24, 2021
@gilles-peskine-arm gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Sep 27, 2021
@gilles-peskine-arm gilles-peskine-arm merged commit aafb21f into Mbed-TLS:development Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clang 12 bignum issue also affects aarch64
3 participants