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

rsa: fix bn_reduce_once_in_place call for rsaz_mod_exp_avx512_x2 #18626

Closed
wants to merge 1 commit into from
Closed

rsa: fix bn_reduce_once_in_place call for rsaz_mod_exp_avx512_x2 #18626

wants to merge 1 commit into from

Conversation

xry111
Copy link
Contributor

@xry111 xry111 commented Jun 22, 2022

bn_reduce_once_in_place expects the number of BN_ULONG, but factor_size
is moduli bit size.

Fixes #18625.

@xry111
rsa: fix bn_reduce_once_in_place call for rsaz_mod_exp_avx512_x2
71ad6a8 
bn_reduce_once_in_place expects the number of BN_ULONG, but factor_size
is moduli bit size.

Fixes openssl#18625.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 22, 2022
t8m
t8m reviewed Jun 22, 2022
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.

I think this would be acceptable with CLA: trivial.

Or even better please submit a CLA according to https://www.openssl.org/policies/cla.html

@t8m t8m added branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch severity: regression The issue/pr is a regression from previous released version labels Jun 22, 2022
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jun 22, 2022
@xry111
Copy link
Contributor Author

xry111 commented Jun 22, 2022

I've sent a signed ICLA.

@alex
Copy link
Contributor

alex commented Jun 22, 2022

Is there a test that should be added for this, or does the OpenSSL test suite already contain one that simply requires AVX-512 to reproduce?

@t8m
Copy link
Member

t8m commented Jun 22, 2022

Is there a test that should be added for this, or does the OpenSSL test suite already contain one that simply requires AVX-512 to reproduce?

The later - there are multiple tests that would reproduce this but unfortunately no CI runner apparently supports AVX-512.

@t8m
Copy link
Member

t8m commented Jun 22, 2022

Actually could be that some of the "random" test failures as in #18594 are caused by this bug.

@t8m t8m added the approval: review pending This pull request needs review by a committer label Jun 22, 2022
@xry111
Copy link
Contributor Author

xry111 commented Jun 22, 2022 •
edited
Loading

Actually could be that some of the "random" test failures as in #18594 are caused by this bug.

I compared this result with the result on my machine and they are almost same. So it looks like that the run was scheduled on a Skylake-SP (or newer model) server in Azure cloud :)

@xry111
Copy link
Contributor Author

xry111 commented Jun 22, 2022

Reopening for the CLA tag.

@xry111 xry111 closed this Jun 22, 2022
@xry111 xry111 reopened this Jun 22, 2022
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 22, 2022
@t8m
Copy link
Member

t8m commented Jun 22, 2022

ping for a second review

@t8m t8m mentioned this pull request Jun 22, 2022
Closed
@t8m t8m added the severity: urgent Fixes an urgent issue (exempt from 24h grace period) label Jun 22, 2022
@t8m
Copy link
Member

t8m commented Jun 22, 2022

Marking it also urgent because this most probably causes many of the CI broken runs seen recently.

@t8m
Copy link
Member

t8m commented Jun 22, 2022

ping @openssl/committers

paulidale
paulidale approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

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

Agreed urgent.

@paulidale
Copy link
Contributor

Merged. Thanks for this fix.

@paulidale paulidale closed this Jun 23, 2022
openssl-machine pushed a commit that referenced this pull request Jun 23, 2022
@xry111 @paulidale
rsa: fix bn_reduce_once_in_place call for rsaz_mod_exp_avx512_x2
4d8a88c 
bn_reduce_once_in_place expects the number of BN_ULONG, but factor_size
is moduli bit size.

Fixes #18625.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #18626)
openssl-machine pushed a commit that referenced this pull request Jun 23, 2022
@xry111 @paulidale
rsa: fix bn_reduce_once_in_place call for rsaz_mod_exp_avx512_x2
a1f7034 
bn_reduce_once_in_place expects the number of BN_ULONG, but factor_size
is moduli bit size.

Fixes #18625.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>

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

(cherry picked from commit 4d8a88c)
@davidben
Copy link
Contributor

Regarding tests, what we've done in BoringSSL to ensure CI coverage of our x86 assembly variants is run everything through Intel SDE. https://www.intel.com/content/www/us/en/developer/articles/tool/software-development-emulator.html

It can simulate CPU models, filling in newer instructions missing on the host and enforcing the lack of various instructions on older CPUs. (Handy for making sure ia32cap checks are correct.) I expect that would have caught this without the flakiness of depending on what kind of CI host you happened to get.

You may also be able to do something similar with QEMU, but I haven't looked into this.

@alyssais alyssais mentioned this pull request Jun 27, 2022
Merged
13 tasks
@bayandin bayandin mentioned this pull request Jun 28, 2022
Closed
6 tasks
vszakats added a commit to curl/curl-for-win that referenced this pull request Jun 29, 2022
@vszakats
openssl 3.0.4 patch AVX512-specific heap buffer overflow
799337d 
Ref: openssl/openssl#18626
Ref: https://github.com/openssl/openssl/commit/71ad6a8da3e39bd4caf5c6c767287ddd9bce8bae.patch?full_index=1"
sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022
@xry111 @sftcd
rsa: fix bn_reduce_once_in_place call for rsaz_mod_exp_avx512_x2
bf9b25f 
bn_reduce_once_in_place expects the number of BN_ULONG, but factor_size
is moduli bit size.

Fixes openssl#18625.

Signed-off-by: Xi Ruoyao <xry111@xry111.site>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#18626)
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: review pending This pull request needs review by a committer branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version severity: urgent Fixes an urgent issue (exempt from 24h grace period) 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.

AVX512-specific heap buffer overflow with 3.0.4 release
6 participants
@xry111 @alex @t8m @paulidale @davidben @openssl-machine