ECDSA related PRs
Hi there, I have two separate but related pull requests I'd like to contribute. They both have to do with ECDSA support. - The simple one is a lack of null-pointer check that can cause a crash in certain situations. Easy peasy. - The less simple one (and hopefully not too controversial) adds an ecdsa verify driver (UCLASS_ECDSA) which utilizes tinycrypt to do the crypto work.
Please advise on how best to proceed. Happy to work within the confines of what works best for the larger group.
Thanks, Bob Wolff
Hi Bob,
On Wed, Feb 21, 2024 at 8:30 AM Bob Wolff bob.wolff68@gmail.com wrote:
Hi there, I have two separate but related pull requests I'd like to contribute. They both have to do with ECDSA support.
- The simple one is a lack of null-pointer check that can cause a crash in
certain situations. Easy peasy.
- The less simple one (and hopefully not too controversial) adds an ecdsa
verify driver (UCLASS_ECDSA) which utilizes tinycrypt to do the crypto work.
Please advise on how best to proceed. Happy to work within the confines of what works best for the larger group.
Please send your patches to the U-Boot mailing list via git send-email as described at: https://docs.u-boot.org/en/latest/develop/sending_patches.html
On Wed, 21 Feb 2024, 11:30 Bob Wolff, bob.wolff68@gmail.com wrote:
Hi there, I have two separate but related pull requests I'd like to contribute. They both have to do with ECDSA support.
- The simple one is a lack of null-pointer check that can cause a crash in
certain situations. Easy peasy.
Just send that one on it's own
- The less simple one (and hopefully not too controversial) adds an ecdsa
verify driver (UCLASS_ECDSA) which utilizes tinycrypt to do the crypto work.
Do we already use tiny crypt in the project, if not things like license need to be taken into account in the context of the GPLv2
Please advise on how best to proceed. Happy to work within the confines of
what works best for the larger group.
Thanks, Bob Wolff
Peter, Thanks for helping lead me down the right path here.
WRT tinycrypt, the license is quite permissive. https://github.com/intel/tinycrypt
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/ change the source files directly for such purposes so that updates could be brought in with greater ease.
Let me know your thoughts.
Thanks, Bob Wolff
On Wed, Feb 21, 2024 at 6:03 AM Peter Robinson pbrobinson@gmail.com wrote:
On Wed, 21 Feb 2024, 11:30 Bob Wolff, bob.wolff68@gmail.com wrote:
Hi there, I have two separate but related pull requests I'd like to contribute. They both have to do with ECDSA support.
- The simple one is a lack of null-pointer check that can cause a crash in
certain situations. Easy peasy.
Just send that one on it's own
- The less simple one (and hopefully not too controversial) adds an ecdsa
verify driver (UCLASS_ECDSA) which utilizes tinycrypt to do the crypto work.
Do we already use tiny crypt in the project, if not things like license need to be taken into account in the context of the GPLv2
Please advise on how best to proceed. Happy to work within the confines of
what works best for the larger group.
Thanks, Bob Wolff
Any thoughts on how to proceed with the issue mentioned about tinycrypt warnings/checks?
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/ change the source files directly for such purposes so that updates could be brought in with greater ease.
On Thu, Feb 22, 2024 at 3:07 PM Bob Wolff bob.wolff68@gmail.com wrote:
Peter, Thanks for helping lead me down the right path here.
WRT tinycrypt, the license is quite permissive. https://github.com/intel/tinycrypt
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/ change the source files directly for such purposes so that updates could be brought in with greater ease.
Let me know your thoughts.
Thanks, Bob Wolff
On Wed, Feb 21, 2024 at 6:03 AM Peter Robinson pbrobinson@gmail.com wrote:
On Wed, 21 Feb 2024, 11:30 Bob Wolff, bob.wolff68@gmail.com wrote:
Hi there, I have two separate but related pull requests I'd like to contribute. They both have to do with ECDSA support.
- The simple one is a lack of null-pointer check that can cause a crash
in certain situations. Easy peasy.
Just send that one on it's own
- The less simple one (and hopefully not too controversial) adds an ecdsa
verify driver (UCLASS_ECDSA) which utilizes tinycrypt to do the crypto work.
Do we already use tiny crypt in the project, if not things like license need to be taken into account in the context of the GPLv2
Please advise on how best to proceed. Happy to work within the confines of
what works best for the larger group.
Thanks, Bob Wolff
Hi Bob,
On Wed, Feb 28, 2024 at 7:14 PM Bob Wolff bob.wolff68@gmail.com wrote:
Any thoughts on how to proceed with the issue mentioned about tinycrypt warnings/checks?
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/ change the source files directly for such purposes so that updates could be brought in with greater ease.
I didn't form any opinion on that, hence asking. Why not to backport existing ECC/ECDSA implementation from Linux kernel (crypto/ecc.c, ./crypto/ecdsa.c), like it was already done for RSA, X509 parser, ASN.1 decoder. Pulling the whole library into the U-Boot source tree only just for ECDSA is a bit overkill IMO.
On Thu, Feb 22, 2024 at 3:07 PM Bob Wolff bob.wolff68@gmail.com wrote:
Peter, Thanks for helping lead me down the right path here.
WRT tinycrypt, the license is quite permissive. https://github.com/intel/tinycrypt
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/ change the source files directly for such purposes so that updates could be brought in with greater ease.
Let me know your thoughts.
Thanks, Bob Wolff
On Wed, Feb 21, 2024 at 6:03 AM Peter Robinson pbrobinson@gmail.com wrote:
On Wed, 21 Feb 2024, 11:30 Bob Wolff, bob.wolff68@gmail.com wrote:
Hi there, I have two separate but related pull requests I'd like to contribute. They both have to do with ECDSA support.
- The simple one is a lack of null-pointer check that can cause a crash
in certain situations. Easy peasy.
Just send that one on it's own
- The less simple one (and hopefully not too controversial) adds an ecdsa
verify driver (UCLASS_ECDSA) which utilizes tinycrypt to do the crypto work.
Do we already use tiny crypt in the project, if not things like license need to be taken into account in the context of the GPLv2
Please advise on how best to proceed. Happy to work within the confines of
what works best for the larger group.
Thanks, Bob Wolff
On Thu, Feb 22, 2024 at 03:07:01PM -0800, Bob Wolff wrote:
Peter, Thanks for helping lead me down the right path here.
WRT tinycrypt, the license is quite permissive. https://github.com/intel/tinycrypt
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/ change the source files directly for such purposes so that updates could be brought in with greater ease.
Yeah. For this kind of thing you wouldn't want to make a bunch of checkpatch changes. They sometimes pull crypto and compression libraries into the Linux kernel pretty much unmodified as well for the same reason.
Igor's proposal about pull this stuff from the Linux kernel instead seems like a good idea though.
regards, dan carpenter
Hey all, I'm not opposed to using the kernel ecdsa.c and have taken a quick look at `ecdsa_verify()` - but I'd love if someone could point me in the right direction for how to set up the context and public key. The akcipher_request structure seems to address both the signature and the digest, but I am not seeing how to take my public key data and get it involved. Any examples of usage, possibly? Doing several google searches did not bear fruit for me.
Thanks, bob
On Wed, Feb 28, 2024 at 10:57 PM Dan Carpenter dan.carpenter@linaro.org wrote:
On Thu, Feb 22, 2024 at 03:07:01PM -0800, Bob Wolff wrote:
Peter, Thanks for helping lead me down the right path here.
WRT tinycrypt, the license is quite permissive. https://github.com/intel/tinycrypt
Also, I'd like your advice - I was thinking for the larger patch that I'd do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support ecdsa verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/
change
the source files directly for such purposes so that updates could be brought in with greater ease.
Yeah. For this kind of thing you wouldn't want to make a bunch of checkpatch changes. They sometimes pull crypto and compression libraries into the Linux kernel pretty much unmodified as well for the same reason.
Igor's proposal about pull this stuff from the Linux kernel instead seems like a good idea though.
regards, dan carpenter
Hi Bob,
On Thu, Mar 7, 2024 at 12:49 AM Bob Wolff bob.wolff68@gmail.com wrote:
Hey all, I'm not opposed to using the kernel ecdsa.c and have taken a quick look at `ecdsa_verify()` - but I'd love if someone could point me in the right direction for how to set up the context and public key. The akcipher_request structure seems to address both the signature and the digest, but I am not seeing how to take my public key data and get it involved. Any examples of usage, possibly? Doing several google searches did not bear fruit for me.
Thanks, bob
I'd start with the image_sign_info/image_region structs definitions in [1] and examples of how they are both filled in RSA signature verification tests in [2], as ecdsa_verify() also uses them.
Then I would extend the existing ECDSA dummy test in [3] by adding some test public key data in DER format, data and encrypted hash of this data, like it's done in [2], so you have the way to test your own UCLASS_ECDSA driver implementation.
And obviously the existing ST UCLASS_ECDSA driver might be a good reference [4].
Hope this helps!
[1] include/image.h [2] test/lib/rsa.c [3] test/dm/ecdsa.c [4] arch/arm/mach-stm32mp/ecdsa_romapi.c
On Wed, Feb 28, 2024 at 10:57 PM Dan Carpenter dan.carpenter@linaro.org wrote:
On Thu, Feb 22, 2024 at 03:07:01PM -0800, Bob Wolff wrote:
Peter, Thanks for helping lead me down the right path here.
WRT tinycrypt, the license is quite permissive. https://github.com/intel/tinycrypt
Also, I'd like your advice - I was thinking for the larger patch that
I'd
do it in two commits. The first would be the addition of the tinycrypt files and the second is the actual changes and additions to support
ecdsa
verification. I doubt that's controversial. However when I run a trial `patman` against the tinycrypt commit, I geta huge number of issues: *checkpatch.pl http://checkpatch.pl found 186 error(s), 380 warning(s), 481 checks(s)*
What's your advice on this? I would tend to think we'd want to /not/
change
the source files directly for such purposes so that updates could be brought in with greater ease.
Yeah. For this kind of thing you wouldn't want to make a bunch of checkpatch changes. They sometimes pull crypto and compression libraries into the Linux kernel pretty much unmodified as well for the same reason.
Igor's proposal about pull this stuff from the Linux kernel instead seems like a good idea though.
regards, dan carpenter
participants (6)
-
Bob Wolff -
Dan Carpenter -
Fabio Estevam -
Igor Opaniuk -
Igor Opaniuk -
Peter Robinson