8000 Fix FIPS static build transform issue on aarch. by bryce-shang · Pull Request #137 · aws/aws-lc · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix FIPS static build transform issue on aarch. #137

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

Jump to bottom
Merged
merged 12 commits into from
Apr 27, 2021
Merged

Fix FIPS static build transform issue on aarch. #137

merged 12 commits into from
Apr 27, 2021

Conversation

bryce-shang
Copy link
Contributor

Issues:

Addresses CryptoAlg-725

Description of changes:

This PR is to fix FIPS static build delocate transform issue on aarch. Specifically, the current delocate missed the offset when transforming below assembly statements

ldr	x3,.Lpoly+24
was transformed to
ldr	x3,.Lpoly_BCM_5

adr	x29,.Lone_mont-64
was transformed to
adr	x29,.Lone_mont_BCM_5

With this PR, awslc build and test can work.

Call-outs:

  • This is POC(Proof of Concept) of FIPS static build on aarch. The POC uses Clang-7. Other compilers are not tested yet.
  • This PR replies the change of Fix FIPS static build parse issue on aarch. #135, which fixed FIPS static build delocate parse issue on aarch.

Testing:

aarch-fips-static-build-test-log.out is attached to CryptoAlg-725.

#!/bin/bash -ex
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: Apache-2.0
export CC=clang-7
export CXX=clang++-7

source tests/ci/common_posix_setup.sh

fips_build_and_test -DFIPS=1  -DCMAKE_BUILD_TYPE=Release

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bryce-shang
Remove 'ep_nistz256_add' function, and change offset calculation.
8d6ad98 
'ep_nistz256_add' in p256-armv8-asm.pl is duplicate to p256-nistz.c. Removing 'ep_nistz256_add' function is 
8000
to avoid delocate.go reporting 'Duplicate symbol found'.

The offset calculation expression change is to avoid 'peg' parser errors because the current delocate.peg is not comprehensive enough to parse all assembly expressions.
@bryce-shang
Add new peg rules to address new assembly expression.
e5df257
@bryce-shang
Change assembly expression.
d55a319
@bryce-shang
Add delocate.peg convert README.
237fd98
@bryce-shang
Remove auto-generated comment.
8ee96f6
@bryce-shang
Add more delocate.peg.go generate commands.
979f40e
@bryce-shang
Fix command.
12cab56
@bryce-shang
Add offset to local symbol in delocate.
0b9e41e
@bryce-shang bryce-shang requested review from nebeid and dkostic April 23, 2021 05:47
@bryce-shang bryce-shang marked this pull request as draft April 23, 2021 05:48
nebeid
nebeid reviewed Apr 23, 2021
View reviewed changes
util/fipstools/delocate/delocate.go
// so local symbols in the file need to get changed with suffix(`BCM` + `index`).
// The change is to avoid potential collides with other files.
// If the offset exists, append the offset.
symbol = symbol + offset
Copy link
Contributor

Choose a reason for hiding this comment

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

Should changed be set to true so that a comment is output before this instruction? (l. 618)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didChange is already assigned to changed

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, I hadn't noticed that.

util/fipstools/delocate/delocate.go
@@ -520,6 +520,12 @@ func (d *delocation) processAarch64Instruction(statement, instruction *node32) (
d.redirectors[symbol] = redirector
symbol = redirector
changed = true
} else if didChange && symbolIsLocal && len(offset) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the first inputFile the compiled bcm.c? Is that why it won't have symbols with offsets that were renamed?
Are we sure that the first if will not be hit by those kinds of symbols?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the first inputFile the compiled bcm.c?

No, it's an archived file -- -a $<TARGET_FILE:bcm_c_generated_asm>

./delocate -a $<TARGET_FILE:bcm_c_generated_asm> -o bcm-delocated.S ${BCM_ASM_PROCESSED_SOURCES}

Is that why it won't have symbols with offsets that were renamed?

Case by case, need to check delocate.go. But for local symbol and the statement matching memRef rule, yes.

Are we sure that the first if will not be hit by those kinds of symbols?

For the first file, didChange is false and the symbol is not change.

8000
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, Bryce.

@bryce-shang @nebeid
Update util/fipstools/delocate/delocate.go
5e70e6e 
Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>
nebeid
nebeid reviewed Apr 26, 2021
View reviewed changes
util/fipstools/delocate/delocate.go
// so local symbols in the file need to get changed with suffix(`BCM` + `index`).
// The change is to avoid potential collides with other files.
// If the offset exists, append the offset.
symbol = symbol + offset
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, I hadn't noticed that.

util/fipstools/delocate/delocate.go
@@ -520,6 +520,12 @@ func (d *delocation) processAarch64Instruction(statement, instruction *node32) (
d.redirectors[symbol] = redirector
symbol = redirector
changed = true
} else if didChange && symbolIsLocal && len(offset) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, Bryce.

bryce-shang and others added 3 commits April 26, 2021 08:34
@bryce-shang @nebeid
Update util/fipstools/delocate/delocate.go
62ffb28 
Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>
@bryce-shang
Merge remote-tracking branch 'upstream/arm-fips-static-poc' into delo…
bf83124 
…cate-arm-transform
@bryce-shang
Update comments.
8000
b3e409c
@nebeid nebeid marked this pull request as ready for review April 27, 2021 12:51
@bryce-shang bryce-shang merged commit 11d89c2 into aws:arm-fips-static-poc Apr 27, 2021
@bryce-shang bryce-shang deleted the delocate-arm-transform branch April 27, 2021 14:30
bryce-shang added a commit that referenced this pull request Apr 28, 2021
@bryce-shang @nebeid
Add FIPS aarch POC build to CI. (#138)
d60b60e 
* Fix FIPS static build parse issue on aarch. (#135)

* Remove 'ep_nistz256_add' function, and change offset calculation.

'ep_nistz256_add' in p256-armv8-asm.pl is duplicate to p256-nistz.c. Removing 'ep_nistz256_add' function is to avoid delocate.go reporting 'Duplicate symbol found'.

The offset calculation expression change is to avoid 'peg' parser errors because the current delocate.peg is not comprehensive enough to parse all assembly expressions.

* Add new peg rules to address new assembly expression.

* Change assembly expression.

* Add delocate.peg convert README.

* Remove auto-generated comment.

* Add more delocate.peg.go generate commands.

* Fix command.

* Update build files in generated-src

* Fix FIPS static build transform issue on aarch. (#137)

* Remove 'ep_nistz256_add' function, and change offset calculation.

'ep_nistz256_add' in p256-armv8-asm.pl is duplicate to p256-nistz.c. Removing 'ep_nistz256_add' function is to avoid delocate.go reporting 'Duplicate symbol found'.

The offset calculation expression change is to avoid 'peg' parser errors because the current delocate.peg is not comprehensive enough to parse all assembly expressions.

* Add new peg rules to address new assembly expression.

* Change assembly expression.

* Add delocate.peg convert README.

* Remove auto-generated comment.

* Add more delocate.peg.go generate commands.

* Fix command.

* Add offset to local symbol in delocate.

* Update util/fipstools/delocate/delocate.go

Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>

* Update util/fipstools/delocate/delocate.go

Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>

* Update comments.

Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>

* Add FIPS build CI on aarch.

* Increase go test timeout.

* Increase more timeout.

* Remove timeout variable because ASAN is not enabled.

* Add comment to trigger CI.

Co-authored-by: Nevine Ebeid <66388554+nebeid@users.noreply.github.com>
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Mar 9, 2022
@SalusaSecondus @juneb
Add list of known differences (aws#137)
20d8596 
Co-authored-by: June Blender <juneb@users.noreply.github.com>
torben-hansen pushed a commit to torben-hansen/aws-lc that referenced this pull request Sep 18, 2024
@aqjune-aws
Merge pull request aws#137 from aqjune-aws/p256-opt
a35eb4b 
Add hybrid `p256_montjadd` and `p256_montjdouble` for Arm, slow multipliers
s2n-bignum original commit: awslabs/s2n-bignum@7ff619c
torben-hansen pushed a commit to torben-hansen/aws-lc that referenced this pull request Sep 18, 2024
@aqjune-aws
Merge pull request aws#137 from aqjune-aws/p256-opt
d9db138 
Add hybrid `p256_montjadd` and `p256_montjdouble` for Arm, slow multipliers
s2n-bignum original commit: awslabs/s2n-bignum@7ff619c

s2n-bignum original commit: awslabs/s2n-bignum@a35eb4b
torben-hansen pushed a commit to torben-hansen/aws-lc that referenced this pull request Sep 19, 2024
@aqjune-aws
Merge pull request aws#137 from aqjune-aws/p256-opt
b99876a 
Add hybrid `p256_montjadd` and `p256_montjdouble` for Arm, slow multipliers
s2n-bignum original commit: awslabs/s2n-bignum@7ff619c
dkostic pushed a commit to dkostic/aws-lc that referenced this pull request Dec 5, 2024
@aqjune-aws
Merge pull request aws#137 from aqjune-aws/p256-opt
7ff619c 
Add hybrid `p256_montjadd` and `p256_montjdouble` for Arm, slow multipliers
dkostic pushed a commit to dkostic/aws-lc that referenced this pull request Dec 10, 2024
@aqjune-aws
Merge pull request aws#137 from aqjune-aws/p256-opt
5793ef6 
Add hybrid `p256_montjadd` and `p256_montjdouble` for Arm, slow multipliers
s2n-bignum original commit: awslabs/s2n-bignum@7ff619c
WillChilds-Klein pushed a commit to WillChilds-Klein/aws-lc that referenced this pull request Jan 8, 2025
@SalusaSecondus
Merge branch 'develop' into evp_factory. Picks up 1.6.0 version
6e67e13 
* develop:
  Remove dead code and bump patch version
  Add logic to generate a classpath file
  Clarify best practices are local to ACCP
  Improve documentation (aws#151)
  Update CHANGELOG for 1.6.0 (aws#148)
  Handle RsaCipher#engineDoFinal with no input bytes (aws#147)
  Validate that AesGcmSpi#engineInit gets non-null key (aws#146)
  Move to OpenSSL 1.1.1j (aws#145)
  Add list of known differences (aws#137)
  Better output size estimates. Fixes aws#135
  Move to openssl 1.1.1i (aws#136)
  Initial commit of the development guide (aws#134)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkostic dkostic dkostic approved these changes

@nebeid nebeid nebeid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bryce-shang @dkostic @nebeid
0