8000 Fix FIPS static build parse issue on aarch. by bryce-shang · Pull Request #135 · 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 parse issue on aarch. #135

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 9 commits into from
Apr 26, 2021
Merged

Fix FIPS static build parse issue on aarch. #135

merged 9 commits into from
Apr 26, 2021

Conversation

bryce-shang
Copy link
Contributor
@bryce-shang bryce-shang commented Apr 21, 2021 •
edited
Loading

Issues:

Addresses CryptoAlg-725

Description of changes:

This PR is to fix FIPS static build delocate parse issue on aarch. Specifically, FIPS static build gets delocate.go involved, which performs two functions -- parse and transform. delocate.go replies on delocate.peg.go, which is generated by some peg rules defined in delocate.peg. These peg rules are not comprehensive enough to parse all assembly expressions. To handle the limits of delocate.go parse, below changes were made:

  • Change assembly expression.
    • Old expression: add sp,sp,#32*(12-4) // difference in stack frames.
    • New expression: add sp,sp,#256 // #256 is from #32*(12-4). difference in stack frames.
  • Add new peg rules in delocate.peg.

Minor change: this PR also added README.md to tell how to generate delocate.peg.go from delocate.peg.

With this PR, awslc build can work. But ECDSA tests still fail due to delocate transform issues, which will be addressed in a separated PR.

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.

Testing:

aarch-fips-static-build-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

run_build -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 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 bryce-shang requested review from nebeid and dkostic April 21, 2021 16:25
dkostic
dkostic reviewed Apr 21, 2021
View reviewed changes
nebeid
nebeid reviewed Apr 21, 2021
View reviewed changes
util/fipstools/delocate/README.md
@@ -0,0 +1,14 @@
### Convert delocate.peg to delocate.peg.go
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

dkostic
dkostic reviewed Apr 22, 2021
View reviewed changes
util/fipstools/delocate/README.md
@@ -0,0 +1,22 @@
### Convert delocate.peg to delocate.peg.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, was the process described in README done before? Also is it specific to ARM?

Copy link
Contributor Author
@bryce-shang bryce-shang Apr 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Not specific to ARM. delocate.peg also has rules for x86. The process is not specified in any README but mentioned in Bssl code history.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please share a link to the BSSL history that mentions this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/google/boringssl/blame/302ef5ee124a123a18b8a2fd9a6b6167f4a0e65a/util/fipstools/delocate.peg.go#L3

If more history is needed, you can track the history tab of delocate related files (delocate.go and so on).

Bssl uses go:generate command, which is released in Go 1.4. I use go build to avoid the Golang update. The build is also mentioned in README.md of peg GitHub repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, I'll take a look

crypto/fipsmodule/ec/asm/p256-armv8-asm.pl
@@ -166,30 +166,6 @@
ret
.size ecp_nistz256_sqr_mont,.-ecp_nistz256_sqr_mont

// void ecp_nistz256_add(BN_ULONG x0[4],const BN_ULONG x1[4],
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to mention in the first review, isn't it a bit weird that before ( before deleting this function) the compiler didn't complain that there are two functions with the same name (this one and the one in p256-nistz.c)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this is weird. I guess the compiler may select the method (static defined in p256-nistz.c) before searching other alternatives. I think we may need a separate SIM to track down.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to o 8000 thers. Learn more.

The reason we removed this function is delocate.go does not allow duplicate symbol -- ecp_nistz256_add.

@bryce-shang bryce-shang mentioned this pull request Apr 23, 2021
Merged
@bryce-shang bryce-shang requested a review from dkostic April 23, 2021 14:53
@bryce-shang bryce-shang requested a review from nebeid April 23, 2021 17:15
@bryce-shang bryce-shang merged commit 6ff9778 into aws:arm-fips-static-poc Apr 26, 2021
@bryce-shang bryce-shang deleted the delocate-parse branch April 28, 2021 04:25
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
Better output size estimates. Fixes aws#135
5c04e5d
torben-hansen pushed a commit to torben-hansen/aws-lc that referenced this pull request Sep 18, 2024
@jargh
Merge pull request aws#135 from jargh/main
06576f0 
P-256 precomputed point scalar multiplication
s2n-bignum original commit: awslabs/s2n-bignum@37c69f1
torben-hansen pushed a commit to torben-hansen/aws-lc that referenced this pull request Sep 18, 2024
@jargh
Merge pull request aws#135 from jargh/main
7e74a7b 
P-256 precomputed point scalar multiplication
s2n-bignum original commit: awslabs/s2n-bignum@37c69f1

s2n-bignum original commit: awslabs/s2n-bignum@06576f0
torben-hansen pushed a commit to torben-hansen/aws-lc that referenced this pull request Sep 19, 2024
@jargh
Merge pull request aws#135 from jargh/main
202d1e6 
P-256 precomputed point scalar multiplication
s2n-bignum original commit: awslabs/s2n-bignum@37c69f1
dkostic pushed a commit to dkostic/aws-lc that referenced this pull request Dec 5, 2024
@jargh
Merge pull request aws#135 from jargh/main
37c69f1 
P-256 precomputed point scalar multiplication
dkostic pushed a commit to dkostic/aws-lc that referenced this pull request Dec 10, 2024
@jargh
Merge pull request aws#135 from jargh/main
afe5902 
P-256 precomputed point scalar multiplication
s2n-bignum original commit: awslabs/s2n-bignum@37c69f1
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