Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1627440 > unrolled thread
| Started by | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| First post | 2017-04-20 15:20 +0200 |
| Last post | 2017-04-24 09:30 +0200 |
| Articles | 17 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-20 15:20 +0200
Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-04-20 15:40 +0200
Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-20 15:50 +0200
Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-04-20 16:10 +0200
Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-23 11:40 +0200
Re: [PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-04-20 15:40 +0200
Re: [PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-20 15:40 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Stephan Müller <smueller@chronox.de> - 2017-04-20 15:50 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-23 11:50 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Stephan Müller <smueller@chronox.de> - 2017-04-23 21:00 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-24 08:10 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Stephan Müller <smueller@chronox.de> - 2017-04-24 08:20 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Stephan Müller <smueller@chronox.de> - 2017-04-24 08:30 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-24 09:10 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Stephan Müller <smueller@chronox.de> - 2017-04-24 09:30 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Gilad Ben-Yossef <gilad@benyossef.com> - 2017-04-24 09:10 +0200
Re: [PATCH v2 6/9] staging: ccree: add FIPS support Stephan Müller <smueller@chronox.de> - 2017-04-24 09:30 +0200
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-20 15:20 +0200 |
| Subject | [PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver |
| Message-ID | <tyka5-XM-3@gated-at.bofh.it> |
Arm TrustZone CryptoCell 700 is a family of cryptographic hardware accelerators. It is supported by a long lived series of out of tree drivers, which I am now in the process of unifying and upstreaming. This is the first drop, supporting the new CryptoCell 712 REE. The code still needs some cleanup before maturing to a proper upstream driver, which I am in the process of doing. However, as discussion of some of the capabilities of the hardware and its application to some dm-crypt and dm-verity features recently took place I though it is better to do this in the open via the staging tree. A Git repository based off of Linux 4.11-rc7 is also available at https://github.com/gby/linux.git branch ccree_v2 for those inclined. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> CC: Binoy Jayan <binoy.jayan@linaro.org> CC: Ofir Drang <ofir.drang@arm.com> CC: Stuart Yoder <stuart.yoder@arm.com> Changes from v1: - Broke up patch set into smaller units for mailing list review as per Greg KH's indication. - Changed DT binding compatible tag as per Mark Rutland suggestion. - Moved DT binding document inside the staging directory and added DT binding review to TODO list as per Mark Rutland's request. Many thanks to all reviewers :-) Gilad Ben-Yossef (9): staging: ccree: introduce CryptoCell HW driver staging: ccree: add ahash support staging: ccree: add skcipher support staging: ccree: add IV generation support staging: ccree: add AEAD support staging: ccree: add FIPS support staging: ccree: add TODO list staging: ccree: add DT bindings for Arm CryptoCell MAINTAINERS: add Gilad BY as ccree maintainer MAINTAINERS | 7 + drivers/staging/Kconfig | 2 + drivers/staging/Makefile | 2 +- .../devicetree/bindings/crypto/arm-cryptocell.txt | 27 + drivers/staging/ccree/Kconfig | 43 + drivers/staging/ccree/Makefile | 3 + drivers/staging/ccree/TODO | 28 + drivers/staging/ccree/bsp.h | 21 + drivers/staging/ccree/cc_bitops.h | 62 + drivers/staging/ccree/cc_crypto_ctx.h | 299 +++ drivers/staging/ccree/cc_hal.h | 35 + drivers/staging/ccree/cc_hw_queue_defs.h | 603 +++++ drivers/staging/ccree/cc_lli_defs.h | 57 + drivers/staging/ccree/cc_pal_log.h | 188 ++ drivers/staging/ccree/cc_pal_log_plat.h | 33 + drivers/staging/ccree/cc_pal_types.h | 97 + drivers/staging/ccree/cc_pal_types_plat.h | 29 + drivers/staging/ccree/cc_regs.h | 106 + drivers/staging/ccree/dx_crys_kernel.h | 180 ++ drivers/staging/ccree/dx_env.h | 224 ++ drivers/staging/ccree/dx_host.h | 155 ++ drivers/staging/ccree/dx_reg_base_host.h | 34 + drivers/staging/ccree/dx_reg_common.h | 26 + drivers/staging/ccree/hash_defs.h | 78 + drivers/staging/ccree/hw_queue_defs_plat.h | 43 + drivers/staging/ccree/ssi_aead.c | 2832 ++++++++++++++++++++ drivers/staging/ccree/ssi_aead.h | 120 + drivers/staging/ccree/ssi_buffer_mgr.c | 1876 +++++++++++++ drivers/staging/ccree/ssi_buffer_mgr.h | 105 + drivers/staging/ccree/ssi_cipher.c | 1503 +++++++++++ drivers/staging/ccree/ssi_cipher.h | 89 + drivers/staging/ccree/ssi_config.h | 61 + drivers/staging/ccree/ssi_driver.c | 557 ++++ drivers/staging/ccree/ssi_driver.h | 228 ++ drivers/staging/ccree/ssi_fips.c | 65 + drivers/staging/ccree/ssi_fips.h | 70 + drivers/staging/ccree/ssi_fips_data.h | 315 +++ drivers/staging/ccree/ssi_fips_ext.c | 96 + drivers/staging/ccree/ssi_fips_ll.c | 1681 ++++++++++++ drivers/staging/ccree/ssi_fips_local.c | 369 +++ drivers/staging/ccree/ssi_fips_local.h | 77 + drivers/staging/ccree/ssi_hash.c | 2751 +++++++++++++++++++ drivers/staging/ccree/ssi_hash.h | 101 + drivers/staging/ccree/ssi_ivgen.c | 301 +++ drivers/staging/ccree/ssi_ivgen.h | 72 + drivers/staging/ccree/ssi_pm.c | 150 ++ drivers/staging/ccree/ssi_pm.h | 46 + drivers/staging/ccree/ssi_pm_ext.c | 60 + drivers/staging/ccree/ssi_pm_ext.h | 33 + drivers/staging/ccree/ssi_request_mgr.c | 713 +++++ drivers/staging/ccree/ssi_request_mgr.h | 60 + drivers/staging/ccree/ssi_sram_mgr.c | 138 + drivers/staging/ccree/ssi_sram_mgr.h | 80 + drivers/staging/ccree/ssi_sysfs.c | 440 +++ drivers/staging/ccree/ssi_sysfs.h | 54 + 55 files changed, 17424 insertions(+), 1 deletion(-) create mode 100644 drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt create mode 100644 drivers/staging/ccree/Kconfig create mode 100644 drivers/staging/ccree/Makefile create mode 100644 drivers/staging/ccree/TODO create mode 100644 drivers/staging/ccree/bsp.h create mode 100644 drivers/staging/ccree/cc_bitops.h create mode 100644 drivers/staging/ccree/cc_crypto_ctx.h create mode 100644 drivers/staging/ccree/cc_hal.h create mode 100644 drivers/staging/ccree/cc_hw_queue_defs.h create mode 100644 drivers/staging/ccree/cc_lli_defs.h create mode 100644 drivers/staging/ccree/cc_pal_log.h create mode 100644 drivers/staging/ccree/cc_pal_log_plat.h create mode 100644 drivers/staging/ccree/cc_pal_types.h create mode 100644 drivers/staging/ccree/cc_pal_types_plat.h create mode 100644 drivers/staging/ccree/cc_regs.h create mode 100644 drivers/staging/ccree/dx_crys_kernel.h create mode 100644 drivers/staging/ccree/dx_env.h create mode 100644 drivers/staging/ccree/dx_host.h create mode 100644 drivers/staging/ccree/dx_reg_base_host.h create mode 100644 drivers/staging/ccree/dx_reg_common.h create mode 100644 drivers/staging/ccree/hash_defs.h create mode 100644 drivers/staging/ccree/hw_queue_defs_plat.h create mode 100644 drivers/staging/ccree/ssi_aead.c create mode 100644 drivers/staging/ccree/ssi_aead.h create mode 100644 drivers/staging/ccree/ssi_buffer_mgr.c create mode 100644 drivers/staging/ccree/ssi_buffer_mgr.h create mode 100644 drivers/staging/ccree/ssi_cipher.c create mode 100644 drivers/staging/ccree/ssi_cipher.h create mode 100644 drivers/staging/ccree/ssi_config.h create mode 100644 drivers/staging/ccree/ssi_driver.c create mode 100644 drivers/staging/ccree/ssi_driver.h create mode 100644 drivers/staging/ccree/ssi_fips.c create mode 100644 drivers/staging/ccree/ssi_fips.h create mode 100644 drivers/staging/ccree/ssi_fips_data.h create mode 100644 drivers/staging/ccree/ssi_fips_ext.c create mode 100644 drivers/staging/ccree/ssi_fips_ll.c create mode 100644 drivers/staging/ccree/ssi_fips_local.c create mode 100644 drivers/staging/ccree/ssi_fips_local.h create mode 100644 drivers/staging/ccree/ssi_hash.c create mode 100644 drivers/staging/ccree/ssi_hash.h create mode 100644 drivers/staging/ccree/ssi_ivgen.c create mode 100644 drivers/staging/ccree/ssi_ivgen.h create mode 100644 drivers/staging/ccree/ssi_pm.c create mode 100644 drivers/staging/ccree/ssi_pm.h create mode 100644 drivers/staging/ccree/ssi_pm_ext.c create mode 100644 drivers/staging/ccree/ssi_pm_ext.h create mode 100644 drivers/staging/ccree/ssi_request_mgr.c create mode 100644 drivers/staging/ccree/ssi_request_mgr.h create mode 100644 drivers/staging/ccree/ssi_sram_mgr.c create mode 100644 drivers/staging/ccree/ssi_sram_mgr.h create mode 100644 drivers/staging/ccree/ssi_sysfs.c create mode 100644 drivers/staging/ccree/ssi_sysfs.h -- 2.1.4
[toc] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-04-20 15:40 +0200 |
| Subject | Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver |
| Message-ID | <tyktr-14Q-9@gated-at.bofh.it> |
| In reply to | #1627440 |
On Thu, Apr 20, 2017 at 04:12:55PM +0300, Gilad Ben-Yossef wrote: > +++ b/drivers/staging/ccree/bsp.h > @@ -0,0 +1,21 @@ > +/* > + * Copyright (C) 2012-2016 ARM Limited or its affiliates. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; either version 2 of the License, or (at your option) > + * any later version. Oh, I have to ask, do you really mean "any later version" here and elsewhere? If so, then your MODULE_LICENSE() marking is wrong, please fix that up, or fix up the license text, I can't take incompatible ones without getting angry emails from legal people sent to me... thanks, greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-20 15:50 +0200 |
| Subject | Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver |
| Message-ID | <tykD8-18r-25@gated-at.bofh.it> |
| In reply to | #1627453 |
On Thu, Apr 20, 2017 at 4:33 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 20, 2017 at 04:12:55PM +0300, Gilad Ben-Yossef wrote: >> +++ b/drivers/staging/ccree/bsp.h >> @@ -0,0 +1,21 @@ >> +/* >> + * Copyright (C) 2012-2016 ARM Limited or its affiliates. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the Free >> + * Software Foundation; either version 2 of the License, or (at your option) >> + * any later version. > > Oh, I have to ask, do you really mean "any later version" here and > elsewhere? > > If so, then your MODULE_LICENSE() marking is wrong, please fix that up, > or fix up the license text, I can't take incompatible ones without > getting angry emails from legal people sent to me... > Thanks for noticing this. The copyright + license notice is a boilerplate I got from the powers that be here. I'll consult internally what is the proper action. I don't want to make legal mad either... :-) Thanks again, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-04-20 16:10 +0200 |
| Subject | Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver |
| Message-ID | <tykWu-1uY-23@gated-at.bofh.it> |
| In reply to | #1627470 |
On Thu, Apr 20, 2017 at 04:40:56PM +0300, Gilad Ben-Yossef wrote: > On Thu, Apr 20, 2017 at 4:33 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Thu, Apr 20, 2017 at 04:12:55PM +0300, Gilad Ben-Yossef wrote: > >> +++ b/drivers/staging/ccree/bsp.h > >> @@ -0,0 +1,21 @@ > >> +/* > >> + * Copyright (C) 2012-2016 ARM Limited or its affiliates. > >> + * > >> + * This program is free software; you can redistribute it and/or modify it > >> + * under the terms of the GNU General Public License as published by the Free > >> + * Software Foundation; either version 2 of the License, or (at your option) > >> + * any later version. > > > > Oh, I have to ask, do you really mean "any later version" here and > > elsewhere? > > > > If so, then your MODULE_LICENSE() marking is wrong, please fix that up, > > or fix up the license text, I can't take incompatible ones without > > getting angry emails from legal people sent to me... > > > > Thanks for noticing this. > > The copyright + license notice is a boilerplate I got from the powers > that be here. > > I'll consult internally what is the proper action. I don't want to > make legal mad either... :-) Ok, I'll drop this patch series then, and wait for an updated one with this fixed up. thanks, greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-23 11:40 +0200 |
| Subject | Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver |
| Message-ID | <tzm9Q-6K4-7@gated-at.bofh.it> |
| In reply to | #1627490 |
Hi, [ Re sending with all recipients this time ... ] On Thu, Apr 20, 2017 at 5:01 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: >> > Oh, I have to ask, do you really mean "any later version" here and >> > elsewhere? >> > >> > If so, then your MODULE_LICENSE() marking is wrong, please fix that up, >> > or fix up the license text, I can't take incompatible ones without >> > getting angry emails from legal people sent to me... >> > >> >> Thanks for noticing this. >> >> The copyright + license notice is a boilerplate I got from the powers >> that be here. >> >> I'll consult internally what is the proper action. I don't want to >> make legal mad either... :-) > > Ok, I'll drop this patch series then, and wait for an updated one with > this fixed up. This issue, along with some others pointed by reviewers, are fixed in v3 of the patch set. I will be happy if you choose to take it into the staging tree and will continue to work to cut down the TODO list. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-04-20 15:40 +0200 |
| Subject | Re: [PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver |
| Message-ID | <tykts-14Q-21@gated-at.bofh.it> |
| In reply to | #1627440 |
On Thu, Apr 20, 2017 at 04:12:54PM +0300, Gilad Ben-Yossef wrote: > Arm TrustZone CryptoCell 700 is a family of cryptographic hardware > accelerators. It is supported by a long lived series of out of tree > drivers, which I am now in the process of unifying and upstreaming. > This is the first drop, supporting the new CryptoCell 712 REE. > > The code still needs some cleanup before maturing to a proper > upstream driver, which I am in the process of doing. However, > as discussion of some of the capabilities of the hardware and > its application to some dm-crypt and dm-verity features recently > took place I though it is better to do this in the open via the > staging tree. > > A Git repository based off of Linux 4.11-rc7 is also available at > https://github.com/gby/linux.git branch ccree_v2 for those inclined. If you want this in staging, I'll be glad to take it, but note then you can't work off of an external repo, as syncing the two is almost impossible and more work than you want to go through. So, as long as this builds properly, want me to queue these up in my tree? thanks, greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-20 15:40 +0200 |
| Message-ID | <tykts-14Q-25@gated-at.bofh.it> |
| In reply to | #1627455 |
On Thu, Apr 20, 2017 at 4:30 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Apr 20, 2017 at 04:12:54PM +0300, Gilad Ben-Yossef wrote: >> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware >> accelerators. It is supported by a long lived series of out of tree >> drivers, which I am now in the process of unifying and upstreaming. >> This is the first drop, supporting the new CryptoCell 712 REE. >> >> The code still needs some cleanup before maturing to a proper >> upstream driver, which I am in the process of doing. However, >> as discussion of some of the capabilities of the hardware and >> its application to some dm-crypt and dm-verity features recently >> took place I though it is better to do this in the open via the >> staging tree. >> >> A Git repository based off of Linux 4.11-rc7 is also available at >> https://github.com/gby/linux.git branch ccree_v2 for those inclined. > > If you want this in staging, I'll be glad to take it, but note then you > can't work off of an external repo, as syncing the two is almost > impossible and more work than you want to go through. Once it's in the staging tree I don't need a separate repo. It was only useful so long as I did not have an upstream tree to point people to. > > So, as long as this builds properly, want me to queue these up in my > tree? Yes, please. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Stephan Müller <smueller@chronox.de> |
|---|---|
| Date | 2017-04-20 15:50 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tykD7-18r-15@gated-at.bofh.it> |
| In reply to | #1627440 |
Am Donnerstag, 20. April 2017, 15:13:00 CEST schrieb Gilad Ben-Yossef:
Hi Gilad,
> +/* The function verifies that tdes keys are not weak.*/
> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen)
> +{
> +#ifdef CCREE_FIPS_SUPPORT
> + tdes_keys_t *tdes_key = (tdes_keys_t*)key;
> +
> + /* verify key1 != key2 and key3 != key2*/
I do not think that this check is necessary. There is no FIPS requirement or
IG that mandates this (unlike the XTS key check).
If there would be such requirement, we would need a common service function
for all TDES implementations
> + if (unlikely( (memcmp((u8*)tdes_key->key1, (u8*)tdes_key->key2,
> sizeof(tdes_key->key1)) == 0) || + (memcmp((u8*)tdes_key->key3,
> (u8*)tdes_key->key2, sizeof(tdes_key->key3)) == 0) )) { +
> return -ENOEXEC;
> + }
> +#endif /* CCREE_FIPS_SUPPORT */
> +
> + return 0;
> +}
> +
> +/* The function verifies that xts keys are not weak.*/
> +static int ssi_fips_verify_xts_keys(const u8 *key, unsigned int keylen)
> +{
> +#ifdef CCREE_FIPS_SUPPORT
> + /* Weak key is define as key that its first half (128/256 lsb)
> equals its second half (128/256 msb) */ + int singleKeySize = keylen
> >> 1;
> +
> + if (unlikely(memcmp(key, &key[singleKeySize], singleKeySize) == 0)) {
> + return -ENOEXEC;
Use xts_check_key.
> +The test vectors were taken from:
> +
> +* AES
> +NIST Special Publication 800-38A 2001 Edition
> +Recommendation for Block Cipher Modes of Operation
> +http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf
> +Appendix F: Example Vectors for Modes of Operation of the AES
> +
> +* AES CTS
> +Advanced Encryption Standard (AES) Encryption for Kerberos 5
> +February 2005
> +https://tools.ietf.org/html/rfc3962#appendix-B
> +B. Sample Test Vectors
> +
> +* AES XTS
> +http://csrc.nist.gov/groups/STM/cavp/#08
> +http://csrc.nist.gov/groups/STM/cavp/documents/aes/XTSTestVectors.zip
> +
> +* AES CMAC
> +http://csrc.nist.gov/groups/STM/cavp/index.html#07
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/cmactestvectors.zip
> +
> +* AES-CCM
> +http://csrc.nist.gov/groups/STM/cavp/#07
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/ccmtestvectors.zip
> +
> +* AES-GCM
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/gcmtestvectors.zip
> +
> +* Triple-DES
> +NIST Special Publication 800-67 January 2012
> +Recommendation for the Triple Data Encryption Algorithm (TDEA) Block Cipher
> +http://csrc.nist.gov/publications/nistpubs/800-67-Rev1/SP-800-67-Rev1.pdf
> +APPENDIX B: EXAMPLE OF TDEA FORWARD AND INVERSE CIPHER OPERATIONS +and
> +http://csrc.nist.gov/groups/STM/cavp/#01
> +http://csrc.nist.gov/groups/STM/cavp/documents/des/tdesmct_intermediate.zip
> +
> +* HASH
> +http://csrc.nist.gov/groups/STM/cavp/#03
> +http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip
> +
> +* HMAC
> +http://csrc.nist.gov/groups/STM/cavp/#07
> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/hmactestvectors.zip
> +
> +*/
Is this test vector business really needed? Why do you think that testmgr.c is
not sufficient? Other successful FIPS validations of the kernel crypto API
managed without such special code.
Also, your entire API seems to implement the approach that if there is a self
test error, you disable the cipher functions, but leave the rest in-tact. The
standard kernel crypto API handling logic is to simply panic the kernel. Is it
really necessary to implement a special case for your driver?
Ciao
Stephan
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-23 11:50 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzmjw-6Nq-9@gated-at.bofh.it> |
| In reply to | #1627468 |
Hi,
Thank you for the review.
On Thu, Apr 20, 2017 at 4:39 PM, Stephan Müller <smueller@chronox.de> wrote:
>> +/* The function verifies that tdes keys are not weak.*/
>> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen)
>> +{
>> +#ifdef CCREE_FIPS_SUPPORT
>> + tdes_keys_t *tdes_key = (tdes_keys_t*)key;
>> +
>> + /* verify key1 != key2 and key3 != key2*/
>
> I do not think that this check is necessary. There is no FIPS requirement or
> IG that mandates this (unlike the XTS key check).
>
> If there would be such requirement, we would need a common service function
> for all TDES implementations
I am not sure. I have forwarded a question internally and based on the
answer will either drop this or add a common function and post a patch
add the check to all 3DES implementation.
This has been added to the staging TODO list for the driver.
>
>> + if (unlikely( (memcmp((u8*)tdes_key->key1, (u8*)tdes_key->key2,
>> sizeof(tdes_key->key1)) == 0) || + (memcmp((u8*)tdes_key->key3,
>> (u8*)tdes_key->key2, sizeof(tdes_key->key3)) == 0) )) { +
>> return -ENOEXEC;
>> + }
>> +#endif /* CCREE_FIPS_SUPPORT */
>> +
>> + return 0;
>> +}
>> +
>> +/* The function verifies that xts keys are not weak.*/
>> +static int ssi_fips_verify_xts_keys(const u8 *key, unsigned int keylen)
>> +{
>> +#ifdef CCREE_FIPS_SUPPORT
>> + /* Weak key is define as key that its first half (128/256 lsb)
>> equals its second half (128/256 msb) */ + int singleKeySize = keylen
>> >> 1;
>> +
>> + if (unlikely(memcmp(key, &key[singleKeySize], singleKeySize) == 0)) {
>> + return -ENOEXEC;
>
> Use xts_check_key.
Will fix. Added to TODO staging list for the driver.
>
>> +The test vectors were taken from:
>> +
>> +* AES
>> +NIST Special Publication 800-38A 2001 Edition
>> +Recommendation for Block Cipher Modes of Operation
>> +http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf
>> +Appendix F: Example Vectors for Modes of Operation of the AES
>> +
>> +* AES CTS
>> +Advanced Encryption Standard (AES) Encryption for Kerberos 5
>> +February 2005
>> +https://tools.ietf.org/html/rfc3962#appendix-B
>> +B. Sample Test Vectors
>> +
>> +* AES XTS
>> +http://csrc.nist.gov/groups/STM/cavp/#08
>> +http://csrc.nist.gov/groups/STM/cavp/documents/aes/XTSTestVectors.zip
>> +
>> +* AES CMAC
>> +http://csrc.nist.gov/groups/STM/cavp/index.html#07
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/cmactestvectors.zip
>> +
>> +* AES-CCM
>> +http://csrc.nist.gov/groups/STM/cavp/#07
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/ccmtestvectors.zip
>> +
>> +* AES-GCM
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/gcmtestvectors.zip
>> +
>> +* Triple-DES
>> +NIST Special Publication 800-67 January 2012
>> +Recommendation for the Triple Data Encryption Algorithm (TDEA) Block Cipher
>> +http://csrc.nist.gov/publications/nistpubs/800-67-Rev1/SP-800-67-Rev1.pdf
>> +APPENDIX B: EXAMPLE OF TDEA FORWARD AND INVERSE CIPHER OPERATIONS +and
>> +http://csrc.nist.gov/groups/STM/cavp/#01
>> +http://csrc.nist.gov/groups/STM/cavp/documents/des/tdesmct_intermediate.zip
>> +
>> +* HASH
>> +http://csrc.nist.gov/groups/STM/cavp/#03
>> +http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip
>> +
>> +* HMAC
>> +http://csrc.nist.gov/groups/STM/cavp/#07
>> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/hmactestvectors.zip
>> +
>> +*/
>
> Is this test vector business really needed? Why do you think that testmgr.c is
> not sufficient? Other successful FIPS validations of the kernel crypto API
> managed without such special code.
That is a very good question. I am guessing this has something to do
to with this driver spending its life out of tree and being maintained
against old kernel versions that may have had some gaps in FIPS
testing and since fixed.
I will review what, if at all, is missing from testmgr and fold those
missing parts (if found) there and drop this from the driver.
>
> Also, your entire API seems to implement the approach that if there is a self
> test error, you disable the cipher functions, but leave the rest in-tact. The
> standard kernel crypto API handling logic is to simply panic the kernel. Is it
> really necessary to implement a special case for your driver?
>
>
No it isn't. What ever the behavior we need it should be added,
pending review of course, to the generic FIPS logic handling.
I do wonder if there is value in alternate behavior of stopping crypto
API on FIPS error rather than a panic though. I will try to get an
explanation why we do it this way.
Handling all these has been added to the driver staging TODO list and
will be handled before it matures into drivers/crypto/
Many thanks for the review!
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Stephan Müller <smueller@chronox.de> |
|---|---|
| Date | 2017-04-23 21:00 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzuTM-3JU-9@gated-at.bofh.it> |
| In reply to | #1628966 |
Am Sonntag, 23. April 2017, 11:48:58 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > I do wonder if there is value in alternate behavior of stopping crypto > API on FIPS error rather than a panic though. I will try to get an > explanation why we do it this way. In FIPS, all crypto function must cease if a self test fails. This can be done by instrumenting the crypto API calls with a check to a global flag or by simply terminating the entire "FIPS module". The panic() is the simplest approach to meet that requirement. Ciao Stephan
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-24 08:10 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzFma-3jI-25@gated-at.bofh.it> |
| In reply to | #1628966 |
On Sun, Apr 23, 2017 at 12:48 PM, Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> Hi,
>
> Thank you for the review.
>
> On Thu, Apr 20, 2017 at 4:39 PM, Stephan Müller <smueller@chronox.de> wrote:
>
>>> +/* The function verifies that tdes keys are not weak.*/
>>> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen)
>>> +{
>>> +#ifdef CCREE_FIPS_SUPPORT
>>> + tdes_keys_t *tdes_key = (tdes_keys_t*)key;
>>> +
>>> + /* verify key1 != key2 and key3 != key2*/
>>
>> I do not think that this check is necessary. There is no FIPS requirement or
>> IG that mandates this (unlike the XTS key check).
>>
>> If there would be such requirement, we would need a common service function
>> for all TDES implementations
Well, it turns out there is and we do :-)
This is from crypto/des_generic.c:
/*
* RFC2451:
*
* For DES-EDE3, there is no known need to reject weak or
* complementation keys. Any weakness is obviated by the use of
* multiple keys.
*
* However, if the first two or last two independent 64-bit keys are
* equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the
* same as DES. Implementers MUST reject keys that exhibit this
* property.
*
*/
int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key,
unsigned int keylen)
However, this does not check that k1 == k3. In this case DES3
becomes 2DES (2-keys TDEA), the use of which was dropped post 2015
by NIST Special Publication 800-131A*.
Would it be acceptable if I offer a patch adding this check to
__des3_ede_setkey()
and use that in the ccree driver?
* http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf
Many thanks,
Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker
"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Stephan Müller <smueller@chronox.de> |
|---|---|
| Date | 2017-04-24 08:20 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzFvP-3nE-9@gated-at.bofh.it> |
| In reply to | #1629219 |
Am Montag, 24. April 2017, 08:06:09 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > > Well, it turns out there is and we do :-) > > This is from crypto/des_generic.c: > > /* > * RFC2451: > * > * For DES-EDE3, there is no known need to reject weak or > * complementation keys. Any weakness is obviated by the use of > * multiple keys. > * > * However, if the first two or last two independent 64-bit keys are > * equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the > * same as DES. Implementers MUST reject keys that exhibit this > * property. > * > */ > int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, > unsigned int keylen) > > However, this does not check that k1 == k3. In this case DES3 > becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 > by NIST Special Publication 800-131A*. It is correct that the RFC wants at least a 2key 3DES. And it is correct that SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS 140-2 does *not* require a technical verification of the 3 keys being not identical. Note, formally, FIPS 140-2 requires that the 3 keys (i.e. all 192 bits) must be obtained from *one* call to a DRBG or KDF (separate independent calls to, say, obtain one key at a time is *not* permitted). Of course, fixing the parity bits is allowed after obtaining the random number. > > Would it be acceptable if I offer a patch adding this check to > __des3_ede_setkey() > and use that in the ccree driver? I am not sure it makes sense as the core requirement is the *one* invocation of the DRBG. Ciao Stephan
[toc] | [prev] | [next] | [standalone]
| From | Stephan Müller <smueller@chronox.de> |
|---|---|
| Date | 2017-04-24 08:30 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzFFw-3rT-15@gated-at.bofh.it> |
| In reply to | #1629223 |
Am Montag, 24. April 2017, 08:16:50 CEST schrieb Stephan Müller: Hi Gilad, > > > > int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, > > > > unsigned int keylen) > > > > However, this does not check that k1 == k3. In this case DES3 > > becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 > > by NIST Special Publication 800-131A*. > > It is correct that the RFC wants at least a 2key 3DES. And it is correct > that SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS > 140-2 does *not* require a technical verification of the 3 keys being not > identical. One side note: we had discussed a patch to this function in the past, see [1]. [1] https://patchwork.kernel.org/patch/8293441/ Ciao Stephan
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-24 09:10 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzGid-3VL-5@gated-at.bofh.it> |
| In reply to | #1629231 |
On Mon, Apr 24, 2017 at 9:21 AM, Stephan Müller <smueller@chronox.de> wrote: > Am Montag, 24. April 2017, 08:16:50 CEST schrieb Stephan Müller: > > Hi Gilad, > >> > >> > int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, >> > >> > unsigned int keylen) >> > >> > However, this does not check that k1 == k3. In this case DES3 >> > becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 >> > by NIST Special Publication 800-131A*. >> >> It is correct that the RFC wants at least a 2key 3DES. And it is correct >> that SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS >> 140-2 does *not* require a technical verification of the 3 keys being not >> identical. > > One side note: we had discussed a patch to this function in the past, see [1]. > > [1] https://patchwork.kernel.org/patch/8293441/ > Thanks, I was not aware of that. I guess we could change the function to indicate that a key is valid for decryption but not encryption and have the implementation limiting based on that if there is an interest in SP800-131A compliance. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Stephan Müller <smueller@chronox.de> |
|---|---|
| Date | 2017-04-24 09:30 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzGBz-41Y-3@gated-at.bofh.it> |
| In reply to | #1629253 |
Am Montag, 24. April 2017, 09:07:45 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > I guess we could change the function to indicate that a key is valid > for decryption but not encryption > and have the implementation limiting based on that if there is an > interest in SP800-131A compliance. I would be delighted to see and review a patch. Ciao Stephan
[toc] | [prev] | [next] | [standalone]
| From | Gilad Ben-Yossef <gilad@benyossef.com> |
|---|---|
| Date | 2017-04-24 09:10 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzGid-3VL-9@gated-at.bofh.it> |
| In reply to | #1629223 |
On Mon, Apr 24, 2017 at 9:16 AM, Stephan Müller <smueller@chronox.de> wrote: > Am Montag, 24. April 2017, 08:06:09 CEST schrieb Gilad Ben-Yossef: > > Hi Gilad, >> >> Well, it turns out there is and we do :-) >> >> This is from crypto/des_generic.c: >> >> /* >> * RFC2451: >> * >> * For DES-EDE3, there is no known need to reject weak or >> * complementation keys. Any weakness is obviated by the use of >> * multiple keys. >> * >> * However, if the first two or last two independent 64-bit keys are >> * equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the >> * same as DES. Implementers MUST reject keys that exhibit this >> * property. >> * >> */ >> int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, >> unsigned int keylen) >> >> However, this does not check that k1 == k3. In this case DES3 >> becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 >> by NIST Special Publication 800-131A*. > > It is correct that the RFC wants at least a 2key 3DES. And it is correct that > SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS 140-2 > does *not* require a technical verification of the 3 keys being not identical. > > Note, formally, FIPS 140-2 requires that the 3 keys (i.e. all 192 bits) must > be obtained from *one* call to a DRBG or KDF (separate independent calls to, > say, obtain one key at a time is *not* permitted). Of course, fixing the > parity bits is allowed after obtaining the random number. >> >> Would it be acceptable if I offer a patch adding this check to >> __des3_ede_setkey() >> and use that in the ccree driver? > > I am not sure it makes sense as the core requirement is the *one* invocation > of the DRBG. Thanks you for the clarification. As I think is obvious by now I am not a FIPS expert by any stretch. Isn't the requirements on DRBG or KDF invocations pertain to key generation only? What happens if you don't derive the keys on the system, but wish to use keys derived elsewhere? I assumed the limitations on weak keys etc. were meant to protect against that scenario and are therefore independent from key generation requirements, but I may have misunderstood that. Anyway, if I understand you correctly, what you are saying is that these checks might make an implementation RFC 2451 and SP800-131A compliant respectively but they are not required for FIPS 140-2 compliance? did I understand that correctly? If so, since two 3DES implementation in the kernel already do the RFC 2451 compliant check I assume you would not object to the ccree driver using the same function, disconnect from FIPS being set or not, just as is being done with the other 3DES implementation. In addition, would it be OK if we did an extra check in the ccree driver for SP800-131A key compliance and disable encryption (but allow decryption) if the key fails the check? again, this would be independent from FIPS mode? Thanks again, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[toc] | [prev] | [next] | [standalone]
| From | Stephan Müller <smueller@chronox.de> |
|---|---|
| Date | 2017-04-24 09:30 +0200 |
| Subject | Re: [PATCH v2 6/9] staging: ccree: add FIPS support |
| Message-ID | <tzGBA-41Y-13@gated-at.bofh.it> |
| In reply to | #1629255 |
Am Montag, 24. April 2017, 09:04:13 CEST schrieb Gilad Ben-Yossef: Hi Gilad, > > Thanks you for the clarification. As I think is obvious by now I am > not a FIPS expert by any stretch. > > Isn't the requirements on DRBG or KDF invocations pertain to key > generation only? > What happens if you don't derive the keys on the system, but wish to > use keys derived elsewhere? > I assumed the limitations on weak keys etc. were meant to protect > against that scenario and are > therefore independent from key generation requirements, but I may have > misunderstood that. That is exactly an important question. NIST lately moved away from a pure cipher-only view of cryptography to a more holistic view (i.e. where are ciphers used). That said, for 3DES there is no formal requirement that the 3 keys must be checked. NIST is fine when documentation says how the keys are generated by some logic outside the module. > > Anyway, if I understand you correctly, what you are saying is that > these checks might make an > implementation RFC 2451 and SP800-131A compliant respectively but they > are not required for > FIPS 140-2 compliance? did I understand that correctly? Correct. Regarding SP800-131A, it only says that a full 3key 3DES is required. It does not say whether/how the key shall be enforced not being identical. > > If so, since two 3DES implementation in the kernel already do the RFC > 2451 compliant check I assume > you would not object to the ccree driver using the same function, > disconnect from FIPS being set or > not, just as is being done with the other 3DES implementation. Absolutely. If possible, all 3DES implementations should use the same checking functions. The existing checking function regarding the prevention of 1key 3DES should be used by your code too. All I am saying that from a FIPS perspective, there is no need to extent the common function by a 3key 3DES enforcer. > > In addition, would it be OK if we did an extra check in the ccree > driver for SP800-131A key compliance > and disable encryption (but allow decryption) if the key fails the > check? again, this would be independent > from FIPS mode? My personal taste would be: changes that makes sense for all 3DES implementations should go to a common function. Otherwise, 3DES implementation A behaves differently than impl. B. That said, having a check that all three keys are non-identical would certainly be good (see my Ack to the patch from a year ago). But you cannot use the argument that FIPS mandates it to push it through. :-) Ciao Stephan PS: There is currently a new requirement being discussed for FIPS: 3DES operations should not allow more than 4GB of data to be encrypted with one key. This requirement would need technical enforcement. I am looking into this one for some time now.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web