You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "xiaoxiang781216 (via GitHub)" <gi...@apache.org> on 2023/08/13 18:45:50 UTC
[GitHub] [nuttx] xiaoxiang781216 opened a new pull request, #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
xiaoxiang781216 opened a new pull request, #10211:
URL: https://github.com/apache/nuttx/pull/10211
## Summary
since it's simpler to let linker remove the unused functions from the final image
## Impact
crypto
## Testing
ci
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1680951879
> Maybe we will need to find an AES implementation under Apache License because AES is very important for many applications such as TLS.
>
> Here: https://github.com/Mbed-TLS/mbedtls/blob/development/library/aes.c
AES in kernel space is just used to very crypto driver framework work as design, nobody will use it in the production since:
1. Implement the real crypto AES driver if hardware support it and enable it in userspace crypto library like:
https://github.com/apache/nuttx-apps/pull/1932
2. Or use the userspace crypto library(e.g. mbedtls, tinycypto) directly
It doesn't make sense to port a software crypto algo to kernel due to license.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1676447669
@xiaoxiang781216 I understand that your idea is to simplify the Kconfig, but I think removing these symbols will cause the compiler to spend time compiling all these files, just to the end the linker decide not to include it in the final binary.
Is there any other benefit of doing it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1680976411
> @xiaoxiang781216 according with LICENSE file only AES is BSD. All NuttX files came from BSD because NuttX was BSD :-)
>
> But they were re-licensed as Apache license. The AES is an exception because it is a foreign file. I think it is better to put CRYPTO_AES as a separated option instead forcing the users to enable BSD License to use the crypto
Ok, I keep CRYPTO_AES as before, @acassis please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 merged pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #10211:
URL: https://github.com/apache/nuttx/pull/10211
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1680213921
@acassis what's your opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] btashton commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "btashton (via GitHub)" <gi...@apache.org>.
btashton commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1681036653
I think the BSD flag is of zero value and should be dropped. It does not make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1680806773
Maybe we will need to find an AES implementation under Apache License because AES is very important for many applications such as TLS
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1676572151
> @xiaoxiang781216 I understand that your idea is to simplify the Kconfig, but I think removing these symbols will cause the compiler to spend time compiling all these files, just to the end the linker decide not to include it in the final binary.
Yes, it's a tradeoff between the build speed and user friendly. I prefer to simplify Kconfig to make the selection as automation as possible if the speed degradation is minor like this case.
>
> Is there any other benefit of doing it?
>
Yes, if you list nuttx/crypto, you can see that we support a dozen of soft crypto algo now. If we follow the original design principle, a dozen of option needs to be added to Kconfig, it's terrible to use them correctly for a newbie.
> Another detail is that some symbols depend on user manually selecting CONFIG_ALLOW_BSD_COMPONENTS, otherwise the final firmware cannot be 100% Apache license compatible
ALLOW_BSD_COMPONENTS should be move to "config CRYPTO" instead, since the whole crypto code base come from FreeBSD.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1677808782
@xiaoxiang781216 done! But I think now we have other issue: you are assuming that all Crypto API depends on BSD license, that is not true case the user are not using AES
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1677797222
@acassis the license issue is fixed, please review again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1709346816
@acassis could you merge this patch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] btashton commented on a diff in pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "btashton (via GitHub)" <gi...@apache.org>.
btashton commented on code in PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#discussion_r1292889522
##########
crypto/Kconfig:
##########
@@ -50,28 +50,9 @@ config CRYPTO_CRYPTODEV_HARDWARE
depends on CRYPTO_CRYPTODEV
default n
-config CRYPTO_SW_AES
- bool "Software AES library"
- depends on ALLOW_BSD_COMPONENTS
Review Comment:
I generally agree here to just let the linker deal with it, but if want to actually have this `ALLOW_BSD_COMPONENTS` flag we cannot merge this in. That said I don't think this flag provides much value and I don't think it guards all BSD code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1680780336
@xiaoxiang781216 according with LICENSE file only AES is BSD. All NuttX files came from BSD because NuttX was BSD :-)
But they were re-licensed as Apache license. The AES is an exception because it is a foreign file. I think it is better to put CRYPTO_AES as a separated option instead forcing the users to enable BSD License to use the crypto
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#discussion_r1292908451
##########
crypto/Kconfig:
##########
@@ -50,28 +50,9 @@ config CRYPTO_CRYPTODEV_HARDWARE
depends on CRYPTO_CRYPTODEV
default n
-config CRYPTO_SW_AES
- bool "Software AES library"
- depends on ALLOW_BSD_COMPONENTS
Review Comment:
Move ALLOW_BSD_COMPONENTS to config CRYPTO, since the whole crpyto driver framework come from FreeBSD.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1696681243
ping @acassis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1681640360
> I think the BSD flag is of zero value and should be dropped. It does not make sense.
As I understand, how to process IP is different from BSD to Apache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1677874045
> @xiaoxiang781216 done! But I think now we have other issue: you are assuming that all Crypto API depends on BSD license, that is not true case the user are not using AES
Since all other software crypto and /dev/crytpo comes from BSD, it's right to let the whole crypto subsystem depends on ALLOW_BSD_COMPONENTS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #10211: crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #10211:
URL: https://github.com/apache/nuttx/pull/10211#issuecomment-1691986490
@acassis do you have more suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org