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.

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

crypto: Remove CRYPTO_SW_AES and CRYPTO_BLAKE2S #10211

Merged
merged 1 commit into from
Sep 10, 2023

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

since it's simpler to let linker remove the unused functions from the final image

Impact

crypto

Testing

ci

@acassis
Copy link
Contributor

acassis commented Aug 13, 2023

@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?

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

crypto/Kconfig Outdated Show resolved Hide resolved
@xiaoxiang781216
Copy link
Contributor Author

@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.

@xiaoxiang781216
Copy link
Contributor Author

@acassis the license issue is fixed, please review again.

@acassis
Copy link
Contributor

acassis commented Aug 14, 2023

@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

@xiaoxiang781216
Copy link
Contributor Author

@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.

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Aug 16, 2023

@acassis what's your opinion?

@acassis
Copy link
Contributor

acassis commented Aug 16, 2023

@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

@acassis
Copy link
Contributor

acassis commented Aug 16, 2023

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

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Aug 16, 2023

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 verify 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:
    mbedtls: add mbedtls alternative implementation modules nuttx-apps#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.

@xiaoxiang781216
Copy link
Contributor Author

@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.

@btashton
Copy link
Contributor

I think the BSD flag is of zero value and should be dropped. It does not make sense.

since it's more simple to let linker remove the unused functions from the final image

Signed-off-by: Xiang Xiao <[email protected]>
@xiaoxiang781216
Copy link
Contributor Author

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.

@xiaoxiang781216
Copy link
Contributor Author

@acassis do you have more suggestion?

@xiaoxiang781216
Copy link
Contributor Author

ping @acassis

@xiaoxiang781216
Copy link
Contributor Author

@acassis could you merge this patch?

@xiaoxiang781216 xiaoxiang781216 merged commit dec99c0 into apache:master Sep 10, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants