lkml.org 
[lkml]   [2021]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 03/18] crypto: dh - optimize domain parameter serialization for well-known groups
    Date
    On 12/9/21 10:03 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 | 90 ++++++++++++++++++++++++++++++++++++---------
    > crypto/testmgr.h | 16 +++++---
    > include/crypto/dh.h | 6 +++
    > 3 files changed, 88 insertions(+), 24 deletions(-)
    >
    > diff --git a/crypto/dh_helper.c b/crypto/dh_helper.c
    > index aabc91e4f63f..9f21204e5dee 100644
    > --- a/crypto/dh_helper.c
    > +++ b/crypto/dh_helper.c
    > @@ -10,7 +10,31 @@
    > #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) + 4 * 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 +52,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)
    > @@ -45,18 +72,24 @@ int crypto_dh_encode_key(char *buf, unsigned int len, const struct dh *params)
    > .type = CRYPTO_KPP_SECRET_TYPE_DH,
    > .len = len
    > };
    > + int group_id;
    >
    > if (unlikely(!len))
    > return -EINVAL;
    >
    > ptr = dh_pack_data(ptr, end, &secret, sizeof(secret));
    > + group_id = (int)params->group_id;
    > + ptr = dh_pack_data(ptr, end, &group_id, sizeof(group_id));

    Me being picky again.
    To my knowledge, 'int' doesn't have a fixed width, but is rather only
    guaranteed to hold certain values.
    So as soon as one relies on any fixed size (as this one does) I tend to
    use fixed size type like 'u32' to make it absolutely clear what is to be
    expected here.

    But the I don't know the conventions in the crypto code; if an 'int' is
    assumed to be 32 bits throughout the crypto code I guess we should be fine.

    Cheers,

    Hannes
    --
    Dr. Hannes Reinecke Kernel Storage Architect
    hare@suse.de +49 911 74053 688
    SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
    HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

    \
     
     \ /
      Last update: 2021-12-10 12:33    [W:3.706 / U:0.588 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site