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

Fixed a bug that the little-endian Microblaze does not work when MBEDTLS_HAVE_ASM is defined #4686

Merged
merged 2 commits into from
Jul 29, 2022
Merged

Fixed a bug that the little-endian Microblaze does not work when MBEDTLS_HAVE_ASM is defined #4686

merged 2 commits into from
Jul 29, 2022

Conversation

Kazuyuki-Kimura
Copy link

@Kazuyuki-Kimura Kazuyuki-Kimura commented Jun 18, 2021 •
edited by daverodgman
Loading

Fixed a bug that the little-endian Microblaze does not work when MBEDTLS_HAVE_ASM is defined.

I accidentally closed #4586.... sorry.

Signed-off-by: Kazuyuki Kimura [email protected]

Fixes #2020

Backport: #6156

@bensze01 bensze01 changed the title fix issue #2020 Fixed a bug that the little-endian Microblaze does not work when MBEDTLS_HAVE_ASM is defined Jun 23, 2021
@bensze01 bensze01 added needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jun 23, 2021
@mpg mpg self-assigned this Jul 1, 2021
gilles-peskine-arm
gilles-peskine-arm reviewed Jul 31, 2021
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I haven't reviewed the code for any other similar issues. I have not run any tests (as you're undoubtedly aware of, we do not have any testing on Microblaze).

Since this is a bug fix, it needs a changelog entry. Can you please create a file ChangeLog.d/bignum-little-endian-microblaze.txt containing something like

Bugfix
   * Fix bignum multiplication (and therefore asymmetric cryptography) on
     little-endian Microblaze when MBEDTLS_HAVE_ASM is enabled. Fixes #2020.

@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Jul 31, 2021
Draft
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Feb 25, 2022
Closed
daverodgman
daverodgman requested changes May 13, 2022
View reviewed changes
Copy link
Contributor

@daverodgman daverodgman left a comment

Choose a reason for hiding this comment

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

Please add a Changelog

@daverodgman
Copy link
Contributor

This addresses the same issue as #5580 - although I think (but am not sure) that 5580 doesn't properly fix for both endiannesses, whereas this clearly has a code path for both big and little endian. So superficially it looks like this might be the preferred fix.

@daverodgman daverodgman mentioned this pull request May 30, 2022
Closed
@daverodgman
Copy link
Contributor

Rebased and Changelog added. @Kazuyuki-Kimura or @Eurecam-Benjamin please could you test and confirm that addresses the issue? Ideally we also need a 2.28 backport. Thanks.

@daverodgman daverodgman added needs-ci Needs to pass CI tests priority-low Low priority - this may not receive review soon labels May 30, 2022
Kazuyuki Kimura and others added 2 commits May 30, 2022 17:55
@daverodgman
fix issue Mbed-TLS#2020
b88dbdd 
Fixed a bug that the little-endian Microblaze does not work when MBEDTLS_HAVE_ASM is defined.

Signed-off-by: Kazuyuki Kimura <[email protected]>
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman
Add Changelog entry
3527880 
Signed-off-by: Dave Rodgman <[email protected]>
@daverodgman daverodgman added historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed needs-ci Needs to pass CI tests labels May 31, 2022
@daverodgman daverodgman mentioned this pull request Jul 29, 2022
Merged
@daverodgman
Copy link
Contributor

Discussed in historical review; we agreed this is OK to merge without being able to properly test.

@daverodgman daverodgman added the needs-ci Needs to pass CI tests label Jul 29, 2022
tom-cosgrove-arm
tom-cosgrove-arm reviewed Jul 29, 2022
View reviewed changes
library/bn_mul.h
"lhui r9, r3, 0 \n\t"
#endif

#define MULADDC_X1_CORE \
Copy link
Contributor

Choose a reason for hiding this comment

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

annoying that the trailing \ doesn't line up, but ah well

tom-cosgrove-arm
tom-cosgrove-arm approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@tom-cosgrove-arm tom-cosgrove-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@daverodgman daverodgman merged commit 919ff15 into Mbed-TLS:development Jul 29, 2022
@alpsayin alpsayin mentioned this pull request Nov 10, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm left review comments

@daverodgman daverodgman daverodgman requested changes

@tom-cosgrove-arm tom-cosgrove-arm tom-cosgrove-arm approved these changes

Assignees

@mpg mpg

Labels
bug historical-reviewed Reviewed & agreed to keep legacy PR/issue needs-backports Backports are missing or are pending review and approval. needs-ci Needs to pass CI tests needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-low Low priority - this may not receive review soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MicroBlaze does not return from mbedtls_pk_parse_key with MBEDTLS_HAVE_ASM defined
6 participants
@Kazuyuki-Kimura @daverodgman @gilles-peskine-arm @tom-cosgrove-arm @mpg @bensze01