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

Add msl to ARMConstantTweak and recognise ldrsw to prevent delocator errors #2177

Merged
merged 9 commits into from
Feb 11, 2025

Conversation

jakemas
Copy link
Contributor

@jakemas jakemas commented Feb 7, 2025

Issues:

Resolves CI failures around delocator in #2175

Description of changes:

The FIPS static builds for ARM are failing CI:

[57%] Building ASM object crypto/fipsmodule/CMakeFiles/bcm_hashunset.dir/bcm-delocated.S.o
bcm.c: Assembler messages:
bcm.c:125130: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:125133: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
bcm.c:130233: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:130236: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
bcm.c:190061: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:190064: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'
bcm.c:206492: Error: shift expression expected at operand 2 -- movi v4.4s,0xf,.Lbcm_redirector_msl,8' bcm.c:206495: Error: shift expression expected at operand 2 -- mvni v3.4s,0x1f,.Lbcm_redirector_msl,8'

This was discussed back in #2096 but not merged due to the PR being closed.

The Arm instruction has a special argument msl (https://developer.arm.com/documentation/100069/0606/SIMD-Vector-Instructions/MOVI--vector-). The delocator is intepreting msl as a function (or global symbol). Since it’s not defined in the fipsmodule, it creates a jump function bcm_redirector_msl outside the fipsmodule that removes a potential relocation. The suffix of bcm_redirector_msl is the original token, which matches msl.

We fix by adding msl to the list of ARMConstantTweak and regenerate the delocate.peg.go file.

After testing this fix, I begin to see a second error in amazonlinux2023_clang15x_aarch_fips:

[339/575] Generating bcm-delocated.S
FAILED: crypto/fipsmodule/bcm-delocated.S /codebuild/output/src1965601223/src/github.com/aws/aws-lc/test_build_dir/crypto/fipsmodule/bcm-delocated.S 
panic: Symbol reference outside of ldr instruction [recovered]
    panic: error while processing "\tldrsw\tx9, [x9, :lo12:ml_dsa_zetas+4]\n" on line 120399: "Symbol reference outside of ldr instruction"

It seems that ldrsw is not recognised as ldr. We already have ldrsw in our code, so we extend the delocate.go to accept ldrsw in addition to ldr.

Call-outs:

Unblocks progress on work in the FIPS module, such as ED25519 and ML-DSA.

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

@jakemas jakemas requested a review from a team as a code owner February 7, 2025 20:53
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.02%. Comparing base (9921cd9) to head (b289bd6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2177   +/-   ##
=======================================
  Coverage   79.01%   79.02%           
=======================================
  Files         612      612           
  Lines      106071   106072    +1     
  Branches    14994    14994           
=======================================
+ Hits        83814    83821    +7     
+ Misses      21604    21598    -6     
  Partials      653      653           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

torben-hansen
torben-hansen previously approved these changes Feb 7, 2025
@jakemas
Copy link
Contributor Author

jakemas commented Feb 7, 2025

Confirmed on #2175 that these additions fix failing CI in c06dc55.

Copy link
Contributor

@andrewhop andrewhop left a comment

Choose a reason for hiding this comment

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

Please update the arm delocator tests to include msl and ldrsw.

andrewhop
andrewhop previously approved these changes Feb 8, 2025
@jakemas jakemas changed the title Add msl to ARMConstantTweak to prevent delocator errors Add msl to ARMConstantTweak and recognise ldrsw to prevent delocator errors Feb 8, 2025
@andrewhop andrewhop merged commit 028cd9f into aws:main Feb 11, 2025
116 of 120 checks passed
justsmth pushed a commit that referenced this pull request Feb 12, 2025
Note: **Merge after #2177 is merged**.

### Issues:
Resolves #CryptoAlg-2826

As part of validating ML-DSA into AWS-LC-FIPS we must include both
`PQDSA` and `ML-DSA` directories into the fipsmodule.

This PR is a repeat of:
- #2095

### Description of changes: 

Much like the series of PRs for ML-KEM we will implement the move into
the FIPS module across split PRs:
- #1828
- #1832
- #1838

Previous PR:
- #2166

This PR is part (2) to move `ML-DSA` from `crypto/ml_dsa/` to
`crypto/fipsmodule/ml_dsa/`.

We did this once before:
- #2095

But had to revert it here due to static fips builds for ARM failing in
CI (CryptoAlg-2899)
- #2104

We are now unblocked by:
- #2177

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants