* Re: [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups @ 2021-12-06 19:01 kernel test robot 0 siblings, 0 replies; 5+ messages in thread From: kernel test robot @ 2021-12-06 19:01 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 2916 bytes --] CC: kbuild-all(a)lists.01.org In-Reply-To: <20211201004858.19831-4-nstange@suse.de> References: <20211201004858.19831-4-nstange@suse.de> TO: Nicolai Stange <nstange@suse.de> TO: Herbert Xu <herbert@gondor.apana.org.au> TO: "David S. Miller" <davem@davemloft.net> CC: netdev(a)vger.kernel.org CC: "Stephan Müller" <smueller@chronox.de> CC: Hannes Reinecke <hare@suse.de> CC: Torsten Duwe <duwe@suse.de> CC: Zaibo Xu <xuzaibo@huawei.com> CC: Giovanni Cabiddu <giovanni.cabiddu@intel.com> CC: David Howells <dhowells@redhat.com> CC: Jarkko Sakkinen <jarkko@kernel.org> Hi Nicolai, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on herbert-cryptodev-2.6/master] [also build test WARNING on herbert-crypto-2.6/master linus/master jmorris-security/next-testing v5.16-rc4 next-20211206] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nicolai-Stange/crypto-dh-infrastructure-for-NVM-in-band-auth-and-FIPS-conformance/20211201-085159 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master :::::: branch date: 6 days ago :::::: commit date: 6 days ago config: arm-randconfig-m031-20211128 (https://download.01.org/0day-ci/archive/20211207/202112070306.wfN6TcdK-lkp(a)intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: crypto/dh_helper.c:32 get_safe_prime_group() warn: we never enter this loop vim +32 crypto/dh_helper.c 3313d2f891c8a3 Nicolai Stange 2021-12-01 26 3313d2f891c8a3 Nicolai Stange 2021-12-01 27 static inline const struct safe_prime_group * 3313d2f891c8a3 Nicolai Stange 2021-12-01 28 get_safe_prime_group(enum dh_group_id group_id) 3313d2f891c8a3 Nicolai Stange 2021-12-01 29 { 3313d2f891c8a3 Nicolai Stange 2021-12-01 30 int i; 3313d2f891c8a3 Nicolai Stange 2021-12-01 31 3313d2f891c8a3 Nicolai Stange 2021-12-01 @32 for (i = 0; i < ARRAY_SIZE(safe_prime_groups); ++i) { 3313d2f891c8a3 Nicolai Stange 2021-12-01 33 if (safe_prime_groups[i].group_id == group_id) 3313d2f891c8a3 Nicolai Stange 2021-12-01 34 return &safe_prime_groups[i]; 3313d2f891c8a3 Nicolai Stange 2021-12-01 35 } 3313d2f891c8a3 Nicolai Stange 2021-12-01 36 3313d2f891c8a3 Nicolai Stange 2021-12-01 37 return NULL; 3313d2f891c8a3 Nicolai Stange 2021-12-01 38 } 802c7f1c84e4b5 Salvatore Benedetto 2016-06-22 39 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups @ 2021-12-02 3:48 kernel test robot 0 siblings, 0 replies; 5+ messages in thread From: kernel test robot @ 2021-12-02 3:48 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 2890 bytes --] CC: kbuild-all(a)lists.01.org In-Reply-To: <20211201004858.19831-4-nstange@suse.de> References: <20211201004858.19831-4-nstange@suse.de> TO: Nicolai Stange <nstange@suse.de> TO: Herbert Xu <herbert@gondor.apana.org.au> TO: "David S. Miller" <davem@davemloft.net> CC: netdev(a)vger.kernel.org CC: "Stephan Müller" <smueller@chronox.de> CC: Hannes Reinecke <hare@suse.de> CC: Torsten Duwe <duwe@suse.de> CC: Zaibo Xu <xuzaibo@huawei.com> CC: Giovanni Cabiddu <giovanni.cabiddu@intel.com> CC: David Howells <dhowells@redhat.com> CC: Jarkko Sakkinen <jarkko@kernel.org> Hi Nicolai, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on herbert-cryptodev-2.6/master] [also build test WARNING on herbert-crypto-2.6/master linus/master v5.16-rc3 next-20211201] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nicolai-Stange/crypto-dh-infrastructure-for-NVM-in-band-auth-and-FIPS-conformance/20211201-085159 base: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master :::::: branch date: 27 hours ago :::::: commit date: 27 hours ago config: arm-randconfig-m031-20211128 (https://download.01.org/0day-ci/archive/20211202/202112021117.YbF1lcor-lkp(a)intel.com/config) compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: crypto/dh_helper.c:32 get_safe_prime_group() warn: we never enter this loop vim +32 crypto/dh_helper.c 3313d2f891c8a3 Nicolai Stange 2021-12-01 26 3313d2f891c8a3 Nicolai Stange 2021-12-01 27 static inline const struct safe_prime_group * 3313d2f891c8a3 Nicolai Stange 2021-12-01 28 get_safe_prime_group(enum dh_group_id group_id) 3313d2f891c8a3 Nicolai Stange 2021-12-01 29 { 3313d2f891c8a3 Nicolai Stange 2021-12-01 30 int i; 3313d2f891c8a3 Nicolai Stange 2021-12-01 31 3313d2f891c8a3 Nicolai Stange 2021-12-01 @32 for (i = 0; i < ARRAY_SIZE(safe_prime_groups); ++i) { 3313d2f891c8a3 Nicolai Stange 2021-12-01 33 if (safe_prime_groups[i].group_id == group_id) 3313d2f891c8a3 Nicolai Stange 2021-12-01 34 return &safe_prime_groups[i]; 3313d2f891c8a3 Nicolai Stange 2021-12-01 35 } 3313d2f891c8a3 Nicolai Stange 2021-12-01 36 3313d2f891c8a3 Nicolai Stange 2021-12-01 37 return NULL; 3313d2f891c8a3 Nicolai Stange 2021-12-01 38 } 802c7f1c84e4b5 Salvatore Benedetto 2016-06-22 39 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 00/18] crypto: dh - infrastructure for NVM in-band auth and FIPS conformance @ 2021-12-01 0:48 Nicolai Stange 2021-12-01 0:48 ` [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups Nicolai Stange 0 siblings, 1 reply; 5+ messages in thread From: Nicolai Stange @ 2021-12-01 0:48 UTC (permalink / raw) To: Herbert Xu, David S. Miller Cc: Stephan Müller, Hannes Reinecke, Torsten Duwe, Zaibo Xu, Giovanni Cabiddu, David Howells, Jarkko Sakkinen, linux-crypto, linux-kernel, qat-linux, keyrings, Nicolai Stange Hi all, first of all, to the people primarily interested in security/keys/, there's a rather trivial change to security/keys/dh.c in patch 2/18. It would be great to get ACKs for that... Hannes' recent work on NVME in-band authentication ([1]) needs access to the RFC 7919 DH group parameters and also some means to generate ephemeral keys. He currently implements both as part of his patchset (patches 3/12 and 8/12). After some internal discussion, we decided to split off the bits needed from crypto/dh into a separate series, i.e. this one here: - for the RFC 7919 DH group parameters, it's undesirable from a performance POV to serialize the well-known domain parameters via crypto_dh_encode_key() just to deserialize them shortly after again, - from an architectural POV, it would be preferrable to have the key generation code in crypto/dh.c rather than in drivers/nvme/, just in analogy to how key generation is supported by crypto/ecdh.c already. Patches 1-13/18 implement all that is needed for the NVME in-band authentication support. Unfortunately, due to the lack of HW, I have not been able to test the changes to the QAT or HPRE drivers (other than mere compile tests). Yet I figured it would be a good idea to have them behave consistently with dh_generic, and so I chose to introduce support for privkey generation to these as well. By coincidence, NIST SP800-56Arev3 compliance effectively requires that the domain parameters are checked against an approved set, which happens to consists of those safe-prime group parameters specified in RFC 7919, among others. Thus, introducing the RFC 7919 parameters to the kernel allows for making the DH implementation to conform to SP800-56Arev3 with only little effort. I used the opportunity to work crypto/dh towards SP800-56Arev3 conformance with the rest of this patch series, i.e. patches 14-18/18. I can split these into another series on its own, if you like. But as they depend on the earlier patches 1-13/18, I sent them alongside for now. This patchset has been tested with and without fips_enabled on x86_64, ppc64le and s390x, the latter being a big endian machine, which is relevant for the new test vectors. Thanks, Nicolai [1] https://lkml.kernel.org/r/20211123123801.73197-1-hare@suse.de Nicolai Stange (18): crypto: dh - remove struct dh's ->q member crypto: dh - constify struct dh's pointer members crypto: dh - optimize domain parameter serialization for well-known groups crypto: dh - introduce RFC 7919 safe-prime groups crypto: testmgr - add DH RFC 7919 ffdhe2048 test vector crypto: dh - introduce RFC 3526 safe-prime groups crypto: testmgr - add DH RFC 3526 modp2048 test vector crypto: testmgr - run only subset of DH vectors based on config crypto: dh - implement private key generation primitive crypto: dh - introduce support for ephemeral key generation to dh-generic crypto: dh - introduce support for ephemeral key generation to hpre driver crypto: dh - introduce support for ephemeral key generation to qat driver crypto: testmgr - add DH test vectors for key generation lib/mpi: export mpi_rshift crypto: dh - store group id in dh-generic's dh_ctx crypto: dh - calculate Q from P for the full public key verification crypto: dh - try to match domain parameters to a known safe-prime group crypto: dh - accept only approved safe-prime groups in FIPS mode crypto/Kconfig | 20 +- crypto/dh.c | 73 +- crypto/dh_helper.c | 691 +++++++++++++++++- crypto/testmgr.h | 342 ++++++++- drivers/crypto/hisilicon/hpre/hpre_crypto.c | 11 + drivers/crypto/qat/qat_common/qat_asym_algs.c | 9 + include/crypto/dh.h | 52 +- lib/mpi/mpi-bit.c | 1 + security/keys/dh.c | 2 +- 9 files changed, 1141 insertions(+), 60 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups 2021-12-01 0:48 [PATCH 00/18] crypto: dh - infrastructure for NVM in-band auth and FIPS conformance Nicolai Stange @ 2021-12-01 0:48 ` Nicolai Stange 2021-12-01 7:17 ` Hannes Reinecke 0 siblings, 1 reply; 5+ messages in thread From: Nicolai Stange @ 2021-12-01 0:48 UTC (permalink / raw) To: Herbert Xu, David S. Miller Cc: Stephan Müller, Hannes Reinecke, Torsten Duwe, Zaibo Xu, Giovanni Cabiddu, David Howells, Jarkko Sakkinen, linux-crypto, linux-kernel, qat-linux, keyrings, Nicolai Stange DH users are supposed to set a struct dh instance's ->p and ->g domain parameters (as well as the secret ->key), serialize the whole struct dh instance via the crypto_dh_encode_key() helper and pass the encoded blob on to the DH's ->set_secret(). All three currently available DH implementations (generic, drivers/crypto/hisilicon/hpre/ and drivers/crypto/qat/) would then proceed to call the crypto_dh_decode_key() helper for unwrapping the encoded struct dh instance again. Up to now, the only DH user has been the keyctl(KEYCTL_DH_COMPUTE) syscall and thus, all domain parameters have been coming from userspace. The domain parameter encoding scheme for DH's ->set_secret() has been a perfectly reasonable approach in this setting and the potential extra copy of ->p and ->g during the encoding phase didn't harm much. However, recently, the need for working with the well-known safe-prime groups' domain parameters from RFC 3526 and RFC 7919 resp. arose from two independent developments: - The NVME in-band authentication support currently being worked on ([1]) needs to install the RFC 7919 ffdhe groups' domain parameters for DH tfms. - In FIPS mode, there's effectively no sensible way for the DH implementation to conform to SP800-56Arev3 other than rejecting any parameter set not corresponding to some approved safe-prime group specified in either of these two RFCs. As the ->p arrays' lengths are in the range from 256 to 1024 bytes, it would be nice if that extra copy during the crypto_dh_encode_key() step from the NVME in-band authentication code could be avoided. Likewise, it would be great if the DH implementation's FIPS handling code could avoid attempting to match the input ->p and ->g against the individual approved groups' parameters via memcmp() if it's known in advance that the input corresponds to such one, as is the case for NVME. Introduce a enum dh_group_id for referring to any of the safe-prime groups known to the kernel. The introduction of actual such safe-prime groups alongside with their resp. P and G parameters will be deferred to later patches. As of now, the new enum contains only a single member, dh_group_id_unknown, which is meant to be associated with parameter sets not corresponding to any of the groups known to the kernel, as is needed to continue to support the current keyctl(KEYCTL_DH_COMPUTE) syscall semantics. Add a new 'group_id' member of type enum group_id to struct dh. Make crypto_dh_encode_key() include it in the serialization and to encode ->p and ->g only if it equals dh_group_id_unknown. For all other possible values of the encoded ->group_id, the receiving decoding primitive, crypto_dh_decode_key(), is made to not decode ->p and ->g from the encoded data, but to look them up in a central registry instead. The intended usage pattern is that users like NVME wouldn't set any of the struct dh's ->p or ->g directly, but only the ->group_id for the group they're interested in. They'd then proceed as usual and call crypto_dh_encode_key() on the struct dh instance, pass the encoded result on to DH's ->set_secret() and the latter would then invoke crypto_dh_decode_key(), which would then in turn lookup the parameters associated with the passed ->group_id. Note that this will avoid the extra copy of the ->p and ->g for the groups (to be made) known to the kernel and also, that a future patch can easily introduce a validation of ->group_id if in FIPS mode. As mentioned above, the introduction of actual safe-prime groups will be deferred to later patches, so for now, only introduce an empty placeholder array safe_prime_groups[] to be queried by crypto_dh_decode_key() for domain parameters associated with a given ->group_id as outlined above. Make its elements to be of the new internal struct safe_prime_group type. Among the members ->group_id, ->p and ->p_size with obvious meaning, there will also be a ->max_strength member for storing the maximum security strength supported by the associated group -- its value will be needed for the upcoming private key generation support. Finally, update the encoded secrets provided by the testmgr's DH test vectors in order to account for the additional ->group_id field expected by crypto_dh_decode_key() now. [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de Signed-off-by: Nicolai Stange <nstange@suse.de> --- crypto/dh_helper.c | 88 +++++++++++++++++++++++++++++++++++---------- crypto/testmgr.h | 16 +++++---- include/crypto/dh.h | 6 ++++ 3 files changed, 86 insertions(+), 24 deletions(-) diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c index aabc91e4f63f..a6c9389d8219 100644 --- a/crypto/dh_helper.c +++ b/crypto/dh_helper.c @@ -10,7 +10,32 @@ #include <crypto/dh.h> #include <crypto/kpp.h> -#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 3 * sizeof(int)) +#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + \ + sizeof(enum dh_group_id) + 3 * sizeof(int)) + +static const struct safe_prime_group +{ + enum dh_group_id group_id; + unsigned int max_strength; + unsigned int p_size; + const char *p; +} safe_prime_groups[] = {}; + +/* 2 is used as a generator for all safe-prime groups. */ +static const char safe_prime_group_g[] = { 2 }; + +static inline const struct safe_prime_group * +get_safe_prime_group(enum dh_group_id group_id) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(safe_prime_groups); ++i) { + if (safe_prime_groups[i].group_id == group_id) + return &safe_prime_groups[i]; + } + + return NULL; +} static inline u8 *dh_pack_data(u8 *dst, u8 *end, const void *src, size_t size) { @@ -28,7 +53,10 @@ static inline const u8 *dh_unpack_data(void *dst, const void *src, size_t size) static inline unsigned int dh_data_size(const struct dh *p) { - return p->key_size + p->p_size + p->g_size; + if (p->group_id == dh_group_id_unknown) + return p->key_size + p->p_size + p->g_size; + else + return p->key_size; } unsigned int crypto_dh_key_len(const struct dh *p) @@ -50,13 +78,18 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params) return -EINVAL; ptr = dh_pack_data(ptr, end, &secret, sizeof(secret)); + ptr = dh_pack_data(ptr, end, ¶ms->group_id, + sizeof(params->group_id)); ptr = dh_pack_data(ptr, end, ¶ms->key_size, sizeof(params->key_size)); ptr = dh_pack_data(ptr, end, ¶ms->p_size, sizeof(params->p_size)); ptr = dh_pack_data(ptr, end, ¶ms->g_size, sizeof(params->g_size)); ptr = dh_pack_data(ptr, end, params->key, params->key_size); - ptr = dh_pack_data(ptr, end, params->p, params->p_size); - ptr = dh_pack_data(ptr, end, params->g, params->g_size); + if (params->group_id == dh_group_id_unknown) { + ptr = dh_pack_data(ptr, end, params->p, params->p_size); + ptr = dh_pack_data(ptr, end, params->g, params->g_size); + } + if (ptr != end) return -EINVAL; return 0; @@ -75,12 +108,45 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) if (secret.type != CRYPTO_KPP_SECRET_TYPE_DH) return -EINVAL; + ptr = dh_unpack_data(¶ms->group_id, ptr, sizeof(params->group_id)); ptr = dh_unpack_data(¶ms->key_size, ptr, sizeof(params->key_size)); ptr = dh_unpack_data(¶ms->p_size, ptr, sizeof(params->p_size)); ptr = dh_unpack_data(¶ms->g_size, ptr, sizeof(params->g_size)); if (secret.len != crypto_dh_key_len(params)) return -EINVAL; + if (params->group_id == dh_group_id_unknown) { + /* Don't allocate memory. Set pointers to data within + * the given buffer + */ + params->key = (void *)ptr; + params->p = (void *)(ptr + params->key_size); + params->g = (void *)(ptr + params->key_size + params->p_size); + + /* + * Don't permit 'p' to be 0. It's not a prime number, + * and it's subject to corner cases such as 'mod 0' + * being undefined or crypto_kpp_maxsize() returning + * 0. + */ + if (memchr_inv(params->p, 0, params->p_size) == NULL) + return -EINVAL; + + } else { + const struct safe_prime_group *g; + + g = get_safe_prime_group(params->group_id); + if (!g) + return -EINVAL; + + params->key = (void *)ptr; + + params->p = g->p; + params->p_size = g->p_size; + params->g = safe_prime_group_g; + params->g_size = sizeof(safe_prime_group_g); + } + /* * Don't permit the buffer for 'key' or 'g' to be larger than 'p', since * some drivers assume otherwise. @@ -89,20 +155,6 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) params->g_size > params->p_size) return -EINVAL; - /* Don't allocate memory. Set pointers to data within - * the given buffer - */ - params->key = (void *)ptr; - params->p = (void *)(ptr + params->key_size); - params->g = (void *)(ptr + params->key_size + params->p_size); - - /* - * Don't permit 'p' to be 0. It's not a prime number, and it's subject - * to corner cases such as 'mod 0' being undefined or - * crypto_kpp_maxsize() returning 0. - */ - if (memchr_inv(params->p, 0, params->p_size) == NULL) - return -EINVAL; return 0; } diff --git a/crypto/testmgr.h b/crypto/testmgr.h index 7f7d5ae48721..a8983c8626fa 100644 --- a/crypto/testmgr.h +++ b/crypto/testmgr.h @@ -1244,13 +1244,15 @@ static const struct kpp_testvec dh_tv_template[] = { .secret = #ifdef __LITTLE_ENDIAN "\x01\x00" /* type */ - "\x11\x02" /* len */ + "\x15\x02" /* len */ + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ "\x00\x01\x00\x00" /* key_size */ "\x00\x01\x00\x00" /* p_size */ "\x01\x00\x00\x00" /* g_size */ #else "\x00\x01" /* type */ - "\x02\x11" /* len */ + "\x02\x15" /* len */ + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ "\x00\x00\x01\x00" /* key_size */ "\x00\x00\x01\x00" /* p_size */ "\x00\x00\x00\x01" /* g_size */ @@ -1342,7 +1344,7 @@ static const struct kpp_testvec dh_tv_template[] = { "\xd3\x34\x49\xad\x64\xa6\xb1\xc0\x59\x28\x75\x60\xa7\x8a\xb0\x11" "\x56\x89\x42\x74\x11\xf5\xf6\x5e\x6f\x16\x54\x6a\xb1\x76\x4d\x50" "\x8a\x68\xc1\x5b\x82\xb9\x0d\x00\x32\x50\xed\x88\x87\x48\x92\x17", - .secret_size = 529, + .secret_size = 533, .b_public_size = 256, .expected_a_public_size = 256, .expected_ss_size = 256, @@ -1351,13 +1353,15 @@ static const struct kpp_testvec dh_tv_template[] = { .secret = #ifdef __LITTLE_ENDIAN "\x01\x00" /* type */ - "\x11\x02" /* len */ + "\x15\x02" /* len */ + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ "\x00\x01\x00\x00" /* key_size */ "\x00\x01\x00\x00" /* p_size */ "\x01\x00\x00\x00" /* g_size */ #else "\x00\x01" /* type */ - "\x02\x11" /* len */ + "\x02\x15" /* len */ + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ "\x00\x00\x01\x00" /* key_size */ "\x00\x00\x01\x00" /* p_size */ "\x00\x00\x00\x01" /* g_size */ @@ -1449,7 +1453,7 @@ static const struct kpp_testvec dh_tv_template[] = { "\x5e\x5a\x64\xbd\xf6\x85\x04\xe8\x28\x6a\xac\xef\xce\x19\x8e\x9a" "\xfe\x75\xc0\x27\x69\xe3\xb3\x7b\x21\xa7\xb1\x16\xa4\x85\x23\xee" "\xb0\x1b\x04\x6e\xbd\xab\x16\xde\xfd\x86\x6b\xa9\x95\xd7\x0b\xfd", - .secret_size = 529, + .secret_size = 533, .b_public_size = 256, .expected_a_public_size = 256, .expected_ss_size = 256, diff --git a/include/crypto/dh.h b/include/crypto/dh.h index 67f3f6bca527..15d8b2dfe4a2 100644 --- a/include/crypto/dh.h +++ b/include/crypto/dh.h @@ -19,6 +19,11 @@ * the KPP API function call of crypto_kpp_set_secret. */ +/** enum dh_group_id - identify well-known domain parameter sets */ +enum dh_group_id { + dh_group_id_unknown = 0, +}; + /** * struct dh - define a DH private key * @@ -30,6 +35,7 @@ * @g_size: Size of DH generator G */ struct dh { + enum dh_group_id group_id; const void *key; const void *p; const void *g; -- 2.26.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups 2021-12-01 0:48 ` [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups Nicolai Stange @ 2021-12-01 7:17 ` Hannes Reinecke 2021-12-09 9:08 ` Nicolai Stange 0 siblings, 1 reply; 5+ messages in thread From: Hannes Reinecke @ 2021-12-01 7:17 UTC (permalink / raw) To: Nicolai Stange, Herbert Xu, David S. Miller Cc: Stephan Müller, Torsten Duwe, Zaibo Xu, Giovanni Cabiddu, David Howells, Jarkko Sakkinen, linux-crypto, linux-kernel, qat-linux, keyrings On 12/1/21 1:48 AM, Nicolai Stange wrote: > DH users are supposed to set a struct dh instance's ->p and ->g domain > parameters (as well as the secret ->key), serialize the whole struct dh > instance via the crypto_dh_encode_key() helper and pass the encoded blob > on to the DH's ->set_secret(). All three currently available DH > implementations (generic, drivers/crypto/hisilicon/hpre/ and > drivers/crypto/qat/) would then proceed to call the crypto_dh_decode_key() > helper for unwrapping the encoded struct dh instance again. > > Up to now, the only DH user has been the keyctl(KEYCTL_DH_COMPUTE) syscall > and thus, all domain parameters have been coming from userspace. The domain > parameter encoding scheme for DH's ->set_secret() has been a perfectly > reasonable approach in this setting and the potential extra copy of ->p > and ->g during the encoding phase didn't harm much. > > However, recently, the need for working with the well-known safe-prime > groups' domain parameters from RFC 3526 and RFC 7919 resp. arose from two > independent developments: > - The NVME in-band authentication support currently being worked on ([1]) > needs to install the RFC 7919 ffdhe groups' domain parameters for DH > tfms. > - In FIPS mode, there's effectively no sensible way for the DH > implementation to conform to SP800-56Arev3 other than rejecting any > parameter set not corresponding to some approved safe-prime group > specified in either of these two RFCs. > > As the ->p arrays' lengths are in the range from 256 to 1024 bytes, it > would be nice if that extra copy during the crypto_dh_encode_key() step > from the NVME in-band authentication code could be avoided. Likewise, it > would be great if the DH implementation's FIPS handling code could avoid > attempting to match the input ->p and ->g against the individual approved > groups' parameters via memcmp() if it's known in advance that the input > corresponds to such one, as is the case for NVME. > > Introduce a enum dh_group_id for referring to any of the safe-prime groups > known to the kernel. The introduction of actual such safe-prime groups > alongside with their resp. P and G parameters will be deferred to later > patches. As of now, the new enum contains only a single member, > dh_group_id_unknown, which is meant to be associated with parameter sets > not corresponding to any of the groups known to the kernel, as is needed > to continue to support the current keyctl(KEYCTL_DH_COMPUTE) syscall > semantics. > > Add a new 'group_id' member of type enum group_id to struct dh. Make > crypto_dh_encode_key() include it in the serialization and to encode > ->p and ->g only if it equals dh_group_id_unknown. For all other possible > values of the encoded ->group_id, the receiving decoding primitive, > crypto_dh_decode_key(), is made to not decode ->p and ->g from the encoded > data, but to look them up in a central registry instead. > > The intended usage pattern is that users like NVME wouldn't set any of > the struct dh's ->p or ->g directly, but only the ->group_id for the group > they're interested in. They'd then proceed as usual and call > crypto_dh_encode_key() on the struct dh instance, pass the encoded result > on to DH's ->set_secret() and the latter would then invoke > crypto_dh_decode_key(), which would then in turn lookup the parameters > associated with the passed ->group_id. > > Note that this will avoid the extra copy of the ->p and ->g for the groups > (to be made) known to the kernel and also, that a future patch can easily > introduce a validation of ->group_id if in FIPS mode. > > As mentioned above, the introduction of actual safe-prime groups will be > deferred to later patches, so for now, only introduce an empty placeholder > array safe_prime_groups[] to be queried by crypto_dh_decode_key() for > domain parameters associated with a given ->group_id as outlined above. > Make its elements to be of the new internal struct safe_prime_group type. > Among the members ->group_id, ->p and ->p_size with obvious meaning, there > will also be a ->max_strength member for storing the maximum security > strength supported by the associated group -- its value will be needed for > the upcoming private key generation support. > > Finally, update the encoded secrets provided by the testmgr's DH test > vectors in order to account for the additional ->group_id field expected > by crypto_dh_decode_key() now. > > [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de > > Signed-off-by: Nicolai Stange <nstange@suse.de> > --- > crypto/dh_helper.c | 88 +++++++++++++++++++++++++++++++++++---------- > crypto/testmgr.h | 16 +++++---- > include/crypto/dh.h | 6 ++++ > 3 files changed, 86 insertions(+), 24 deletions(-) > > diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c > index aabc91e4f63f..a6c9389d8219 100644 > --- a/crypto/dh_helper.c > +++ b/crypto/dh_helper.c > @@ -10,7 +10,32 @@ > #include <crypto/dh.h> > #include <crypto/kpp.h> > > -#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 3 * sizeof(int)) > +#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + \ > + sizeof(enum dh_group_id) + 3 * sizeof(int)) That is not a good practise; 'enum' doesn't have a defined size, and will typically default to 'unsigned int'. But this might well be compiler dependent, so I suggest using a fixes size here. > + > +static const struct safe_prime_group > +{ > + enum dh_group_id group_id; > + unsigned int max_strength; > + unsigned int p_size; > + const char *p; > +} safe_prime_groups[] = {}; > + > +/* 2 is used as a generator for all safe-prime groups. */ > +static const char safe_prime_group_g[] = { 2 }; > + > +static inline const struct safe_prime_group * > +get_safe_prime_group(enum dh_group_id group_id) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(safe_prime_groups); ++i) { > + if (safe_prime_groups[i].group_id == group_id) > + return &safe_prime_groups[i]; > + } > + > + return NULL; > +} > > static inline u8 *dh_pack_data(u8 *dst, u8 *end, const void *src, size_t size) > { > @@ -28,7 +53,10 @@ static inline const u8 *dh_unpack_data(void *dst, const void *src, size_t size) > > static inline unsigned int dh_data_size(const struct dh *p) > { > - return p->key_size + p->p_size + p->g_size; > + if (p->group_id == dh_group_id_unknown) > + return p->key_size + p->p_size + p->g_size; > + else > + return p->key_size; > } > > unsigned int crypto_dh_key_len(const struct dh *p) > @@ -50,13 +78,18 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params) > return -EINVAL; > > ptr = dh_pack_data(ptr, end, &secret, sizeof(secret)); > + ptr = dh_pack_data(ptr, end, ¶ms->group_id, > + sizeof(params->group_id)); > ptr = dh_pack_data(ptr, end, ¶ms->key_size, > sizeof(params->key_size)); > ptr = dh_pack_data(ptr, end, ¶ms->p_size, sizeof(params->p_size)); > ptr = dh_pack_data(ptr, end, ¶ms->g_size, sizeof(params->g_size)); > ptr = dh_pack_data(ptr, end, params->key, params->key_size); > - ptr = dh_pack_data(ptr, end, params->p, params->p_size); > - ptr = dh_pack_data(ptr, end, params->g, params->g_size); > + if (params->group_id == dh_group_id_unknown) { > + ptr = dh_pack_data(ptr, end, params->p, params->p_size); > + ptr = dh_pack_data(ptr, end, params->g, params->g_size); > + } > + > if (ptr != end) > return -EINVAL; > return 0; > @@ -75,12 +108,45 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) > if (secret.type != CRYPTO_KPP_SECRET_TYPE_DH) > return -EINVAL; > > + ptr = dh_unpack_data(¶ms->group_id, ptr, sizeof(params->group_id)); > ptr = dh_unpack_data(¶ms->key_size, ptr, sizeof(params->key_size)); > ptr = dh_unpack_data(¶ms->p_size, ptr, sizeof(params->p_size)); > ptr = dh_unpack_data(¶ms->g_size, ptr, sizeof(params->g_size)); > if (secret.len != crypto_dh_key_len(params)) > return -EINVAL; > > + if (params->group_id == dh_group_id_unknown) { > + /* Don't allocate memory. Set pointers to data within > + * the given buffer > + */ > + params->key = (void *)ptr; > + params->p = (void *)(ptr + params->key_size); > + params->g = (void *)(ptr + params->key_size + params->p_size); > + > + /* > + * Don't permit 'p' to be 0. It's not a prime number, > + * and it's subject to corner cases such as 'mod 0' > + * being undefined or crypto_kpp_maxsize() returning > + * 0. > + */ > + if (memchr_inv(params->p, 0, params->p_size) == NULL) > + return -EINVAL; > + > + } else { > + const struct safe_prime_group *g; > + > + g = get_safe_prime_group(params->group_id); > + if (!g) > + return -EINVAL; > + > + params->key = (void *)ptr; > + > + params->p = g->p; > + params->p_size = g->p_size; > + params->g = safe_prime_group_g; > + params->g_size = sizeof(safe_prime_group_g); > + } > + > /* > * Don't permit the buffer for 'key' or 'g' to be larger than 'p', since > * some drivers assume otherwise. > @@ -89,20 +155,6 @@ int crypto_dh_decode_key(const char *buf, unsigned int len, struct dh *params) > params->g_size > params->p_size) > return -EINVAL; > > - /* Don't allocate memory. Set pointers to data within > - * the given buffer > - */ > - params->key = (void *)ptr; > - params->p = (void *)(ptr + params->key_size); > - params->g = (void *)(ptr + params->key_size + params->p_size); > - > - /* > - * Don't permit 'p' to be 0. It's not a prime number, and it's subject > - * to corner cases such as 'mod 0' being undefined or > - * crypto_kpp_maxsize() returning 0. > - */ > - if (memchr_inv(params->p, 0, params->p_size) == NULL) > - return -EINVAL; > > return 0; > } > diff --git a/crypto/testmgr.h b/crypto/testmgr.h > index 7f7d5ae48721..a8983c8626fa 100644 > --- a/crypto/testmgr.h > +++ b/crypto/testmgr.h > @@ -1244,13 +1244,15 @@ static const struct kpp_testvec dh_tv_template[] = { > .secret = > #ifdef __LITTLE_ENDIAN > "\x01\x00" /* type */ > - "\x11\x02" /* len */ > + "\x15\x02" /* len */ > + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ > "\x00\x01\x00\x00" /* key_size */ > "\x00\x01\x00\x00" /* p_size */ > "\x01\x00\x00\x00" /* g_size */ > #else > "\x00\x01" /* type */ > - "\x02\x11" /* len */ > + "\x02\x15" /* len */ > + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ > "\x00\x00\x01\x00" /* key_size */ > "\x00\x00\x01\x00" /* p_size */ > "\x00\x00\x00\x01" /* g_size */ > @@ -1342,7 +1344,7 @@ static const struct kpp_testvec dh_tv_template[] = { > "\xd3\x34\x49\xad\x64\xa6\xb1\xc0\x59\x28\x75\x60\xa7\x8a\xb0\x11" > "\x56\x89\x42\x74\x11\xf5\xf6\x5e\x6f\x16\x54\x6a\xb1\x76\x4d\x50" > "\x8a\x68\xc1\x5b\x82\xb9\x0d\x00\x32\x50\xed\x88\x87\x48\x92\x17", > - .secret_size = 529, > + .secret_size = 533, > .b_public_size = 256, > .expected_a_public_size = 256, > .expected_ss_size = 256, > @@ -1351,13 +1353,15 @@ static const struct kpp_testvec dh_tv_template[] = { > .secret = > #ifdef __LITTLE_ENDIAN > "\x01\x00" /* type */ > - "\x11\x02" /* len */ > + "\x15\x02" /* len */ > + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ > "\x00\x01\x00\x00" /* key_size */ > "\x00\x01\x00\x00" /* p_size */ > "\x01\x00\x00\x00" /* g_size */ > #else > "\x00\x01" /* type */ > - "\x02\x11" /* len */ > + "\x02\x15" /* len */ > + "\x00\x00\x00\x00" /* group_id == dh_group_id_unknown */ > "\x00\x00\x01\x00" /* key_size */ > "\x00\x00\x01\x00" /* p_size */ > "\x00\x00\x00\x01" /* g_size */ > @@ -1449,7 +1453,7 @@ static const struct kpp_testvec dh_tv_template[] = { > "\x5e\x5a\x64\xbd\xf6\x85\x04\xe8\x28\x6a\xac\xef\xce\x19\x8e\x9a" > "\xfe\x75\xc0\x27\x69\xe3\xb3\x7b\x21\xa7\xb1\x16\xa4\x85\x23\xee" > "\xb0\x1b\x04\x6e\xbd\xab\x16\xde\xfd\x86\x6b\xa9\x95\xd7\x0b\xfd", > - .secret_size = 529, > + .secret_size = 533, > .b_public_size = 256, > .expected_a_public_size = 256, > .expected_ss_size = 256, > diff --git a/include/crypto/dh.h b/include/crypto/dh.h > index 67f3f6bca527..15d8b2dfe4a2 100644 > --- a/include/crypto/dh.h > +++ b/include/crypto/dh.h > @@ -19,6 +19,11 @@ > * the KPP API function call of crypto_kpp_set_secret. > */ > > +/** enum dh_group_id - identify well-known domain parameter sets */ > +enum dh_group_id { > + dh_group_id_unknown = 0, > +}; > + Shouldn't the enum definitions be in uppercase? > /** > * struct dh - define a DH private key > * > @@ -30,6 +35,7 @@ > * @g_size: Size of DH generator G > */ > struct dh { > + enum dh_group_id group_id; > const void *key; > const void *p; > const void *g; > Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups 2021-12-01 7:17 ` Hannes Reinecke @ 2021-12-09 9:08 ` Nicolai Stange 0 siblings, 0 replies; 5+ messages in thread From: Nicolai Stange @ 2021-12-09 9:08 UTC (permalink / raw) To: Hannes Reinecke Cc: Nicolai Stange, Herbert Xu, David S. Miller, Stephan Müller, Torsten Duwe, Zaibo Xu, Giovanni Cabiddu, David Howells, Jarkko Sakkinen, linux-crypto, linux-kernel, qat-linux, keyrings Hannes Reinecke <hare@suse.de> writes: > On 12/1/21 1:48 AM, Nicolai Stange wrote: >> diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c >> index aabc91e4f63f..a6c9389d8219 100644 >> --- a/crypto/dh_helper.c >> +++ b/crypto/dh_helper.c >> @@ -10,7 +10,32 @@ >> #include <crypto/dh.h> >> #include <crypto/kpp.h> >> -#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + 3 * >> sizeof(int)) >> +#define DH_KPP_SECRET_MIN_SIZE (sizeof(struct kpp_secret) + \ >> + sizeof(enum dh_group_id) + 3 * sizeof(int)) > > That is not a good practise; 'enum' doesn't have a defined size, and > will typically default to 'unsigned int'. > But this might well be compiler dependent, so I suggest using a fixes > size here. Good point, in particular as a certain encoding is assumed for the test vectors. Changed in v2. Thanks, Nicolai -- SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg), GF: Ivo Totev ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-09 9:08 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-06 19:01 [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2021-12-02 3:48 kernel test robot 2021-12-01 0:48 [PATCH 00/18] crypto: dh - infrastructure for NVM in-band auth and FIPS conformance Nicolai Stange 2021-12-01 0:48 ` [PATCH 03/18] crypto: dh - optimize domain parameter serialization for well-known groups Nicolai Stange 2021-12-01 7:17 ` Hannes Reinecke 2021-12-09 9:08 ` Nicolai Stange
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.