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

Jump to bottom

Fix SHA3_squeeze on armv4. #22719

Closed
wants to merge 1 commit into from
Closed

Fix SHA3_squeeze on armv4. #22719

wants to merge 1 commit into from

Conversation

slontis
Copy link
Member

@slontis slontis commented Nov 14, 2023 •
edited

For armv4 - Only the first 4 parameters can be passed via registers (r0..r3).

As all of the general registers are already used,
r11 was used to store the 'next' param.
R11 is now pushed/poped on entry/exit.

Tested using the same qemu settings as our CI uses.

Checklist
  • documentation is added or updated
  • tests are added or updated

@slontis
Fix SHA3_squeeze on armv4.
c530cb0 
For armv4 - Only the first 4 parameters can be passed via registers
(r0..r3).

As all of the general registers are already used,
r11 was used to store the 'next' param.
R11 is now pushed/poped on entry/exit.
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 14, 2023
@slontis
Copy link
Member Author

slontis commented Nov 14, 2023

See also #22722

@slontis
Copy link
Member Author

slontis commented Nov 14, 2023

Partially fixes #22692

@slontis slontis added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Nov 14, 2023
@t8m t8m added triaged: bug The issue/pr is/fixes a bug severity: regression The issue/pr is a regression from previous released version labels Nov 14, 2023
t8m
t8m approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Assuming you've tested this with qemu.

@t8m t8m added tests: exempted The PR is exempt from requirements for testing and removed approval: otc review pending This pull request needs review by an OTC member labels Nov 14, 2023
@slontis
Copy link
Member Author

slontis commented Nov 14, 2023

Yes, also ran evp_test and evp_xof_test..

@paulidale paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Nov 14, 2023
@zorrorffm
Copy link
Contributor

The fix Looks good to me!

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m
Copy link
Member

t8m commented Nov 15, 2023

Merged to the master branch. Thank you.

@t8m t8m closed this Nov 15, 2023
openssl-machine pushed a commit that referenced this pull request Nov 15, 2023
@slontis @t8m
Fix SHA3_squeeze on armv4.
f5a63bf 
For armv4 - Only the first 4 parameters can be passed via registers
(r0..r3).

As all of the general registers are already used,
r11 was used to store the 'next' param.
R11 is now pushed/poped on entry/exit.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #22719)
@t8m
Copy link
Member

t8m commented Nov 16, 2023

@slontis unfortunately it still did not fix the problem completely as seen in the evp_test failures here:

https://github.com/openssl/openssl/actions/runs/6874516007/job/18696334986
https://github.com/openssl/openssl/actions/runs/6874516007/job/18696335508

@slontis
Copy link
Member Author

slontis commented Nov 16, 2023

Well that is embarrassing. I did run this test and get it to work, but I must of not rerun this after I did some cleanups..
Stack was off by 1. verified this by writing $next to the output buffer and confirmed it was 01 at the start and 00 later.

wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 20, 2023
@slontis
Fix SHA3_squeeze on armv4.
30e7227 
For armv4 - Only the first 4 parameters can be passed via registers
(r0..r3).

As all of the general registers are already used,
r11 was used to store the 'next' param.
R11 is now pushed/poped on entry/exit.

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl/openssl#22719)

Signed-off-by: fly2x <fly2x@hitls.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@paulidale paulidale paulidale approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@slontis @zorrorffm @openssl-machine @t8m @paulidale