| Message ID | 20211201004858.19831-11-nstange@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: dh - infrastructure for NVM in-band auth and FIPS conformance | expand |
On 12/1/21 1:48 AM, Nicolai Stange wrote: > The support for NVME in-band authentication currently in the works ([1]) > needs to generate ephemeral DH keys. Make dh-generic's ->set_secret() > to generate an ephemeral key via the recently added crypto_dh_gen_privkey() > in case the input ->key_size is zero. Note that this behaviour is in > analogy to ecdh's ->set_secret(). > > [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de > > Signed-off-by: Nicolai Stange <nstange@suse.de> > --- > crypto/dh.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
Am Mittwoch, 1. Dezember 2021, 01:48:50 CET schrieb Nicolai Stange: Hi Nicolai, > The support for NVME in-band authentication currently in the works ([1]) > needs to generate ephemeral DH keys. Make dh-generic's ->set_secret() > to generate an ephemeral key via the recently added crypto_dh_gen_privkey() > in case the input ->key_size is zero. Note that this behaviour is in > analogy to ecdh's ->set_secret(). > > [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de > > Signed-off-by: Nicolai Stange <nstange@suse.de> > --- > crypto/dh.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/crypto/dh.c b/crypto/dh.c > index 131b80064cb1..2e49b114e038 100644 > --- a/crypto/dh.c > +++ b/crypto/dh.c > @@ -71,25 +71,41 @@ static int dh_set_secret(struct crypto_kpp *tfm, const > void *buf, { > struct dh_ctx *ctx = dh_get_ctx(tfm); > struct dh params; > + char key[CRYPTO_DH_MAX_PRIVKEY_SIZE]; > + int err; > > /* Free the old MPI key if any */ > dh_clear_ctx(ctx); > > - if (crypto_dh_decode_key(buf, len, ¶ms) < 0) > + err = crypto_dh_decode_key(buf, len, ¶ms); > + if (err) > goto err_clear_ctx; > > - if (dh_set_params(ctx, ¶ms) < 0) > + if (!params.key_size) { As this params data may come from user space, shouldn't we use the same logic as in ecdh's set_key function: if (!params.key || !params.key_size) ? > + err = crypto_dh_gen_privkey(params.group_id, key, > + ¶ms.key_size); > + if (err) > + goto err_clear_ctx; > + params.key = key; > + } > + > + err = dh_set_params(ctx, ¶ms); > + if (err) > goto err_clear_ctx; > > ctx->xa = mpi_read_raw_data(params.key, params.key_size); > - if (!ctx->xa) > + if (!ctx->xa) { > + err = -EINVAL; > goto err_clear_ctx; > + } > + > + memzero_explicit(key, sizeof(key)); > > return 0; > > err_clear_ctx: > dh_clear_ctx(ctx); > - return -EINVAL; > + return err; > } > > /* Ciao Stephan
Stephan Müller <smueller@chronox.de> writes: > Am Mittwoch, 1. Dezember 2021, 01:48:50 CET schrieb Nicolai Stange: > > Hi Nicolai, > >> The support for NVME in-band authentication currently in the works ([1]) >> needs to generate ephemeral DH keys. Make dh-generic's ->set_secret() >> to generate an ephemeral key via the recently added crypto_dh_gen_privkey() >> in case the input ->key_size is zero. Note that this behaviour is in >> analogy to ecdh's ->set_secret(). >> >> [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de >> >> Signed-off-by: Nicolai Stange <nstange@suse.de> >> --- >> crypto/dh.c | 24 ++++++++++++++++++++---- >> 1 file changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/crypto/dh.c b/crypto/dh.c >> index 131b80064cb1..2e49b114e038 100644 >> --- a/crypto/dh.c >> +++ b/crypto/dh.c >> @@ -71,25 +71,41 @@ static int dh_set_secret(struct crypto_kpp *tfm, const >> void *buf, { >> struct dh_ctx *ctx = dh_get_ctx(tfm); >> struct dh params; >> + char key[CRYPTO_DH_MAX_PRIVKEY_SIZE]; >> + int err; >> >> /* Free the old MPI key if any */ >> dh_clear_ctx(ctx); >> >> - if (crypto_dh_decode_key(buf, len, ¶ms) < 0) >> + err = crypto_dh_decode_key(buf, len, ¶ms); >> + if (err) >> goto err_clear_ctx; >> >> - if (dh_set_params(ctx, ¶ms) < 0) >> + if (!params.key_size) { > > As this params data may come from user space, shouldn't we use the same logic > as in ecdh's set_key function: > > if (!params.key || !params.key_size) crypto_dh_decode_key() always leaves params.key set even for !params.key_size, so checking for !params.key wouldn't buy anything here. FWIW, it seems like it's actually the same for crypto_ecdh_decode_key(). I'd personally prefer to not add the !params.key check, because it would suggest that there are code paths which can lead to the condition params.key_size && !params.key. I would find this confusing when reading the code, but OTOH I don't have strong objections, so if you insist on adding the !params.key check, I'd be Ok with it. Thanks, Nicolai > > ? > > >> + err = crypto_dh_gen_privkey(params.group_id, key, >> + ¶ms.key_size); >> + if (err) >> + goto err_clear_ctx; >> + params.key = key; >> + } >> + >> + err = dh_set_params(ctx, ¶ms); >> + if (err) >> goto err_clear_ctx; >> >> ctx->xa = mpi_read_raw_data(params.key, params.key_size); >> - if (!ctx->xa) >> + if (!ctx->xa) { >> + err = -EINVAL; >> goto err_clear_ctx; >> + } >> + >> + memzero_explicit(key, sizeof(key)); >> >> return 0; >> >> err_clear_ctx: >> dh_clear_ctx(ctx); >> - return -EINVAL; >> + return err; >> } >> >> /* > > > Ciao > Stephan > >
diff --git a/crypto/dh.c b/crypto/dh.c index 131b80064cb1..2e49b114e038 100644 --- a/crypto/dh.c +++ b/crypto/dh.c @@ -71,25 +71,41 @@ static int dh_set_secret(struct crypto_kpp *tfm, const void *buf, { struct dh_ctx *ctx = dh_get_ctx(tfm); struct dh params; + char key[CRYPTO_DH_MAX_PRIVKEY_SIZE]; + int err; /* Free the old MPI key if any */ dh_clear_ctx(ctx); - if (crypto_dh_decode_key(buf, len, ¶ms) < 0) + err = crypto_dh_decode_key(buf, len, ¶ms); + if (err) goto err_clear_ctx; - if (dh_set_params(ctx, ¶ms) < 0) + if (!params.key_size) { + err = crypto_dh_gen_privkey(params.group_id, key, + ¶ms.key_size); + if (err) + goto err_clear_ctx; + params.key = key; + } + + err = dh_set_params(ctx, ¶ms); + if (err) goto err_clear_ctx; ctx->xa = mpi_read_raw_data(params.key, params.key_size); - if (!ctx->xa) + if (!ctx->xa) { + err = -EINVAL; goto err_clear_ctx; + } + + memzero_explicit(key, sizeof(key)); return 0; err_clear_ctx: dh_clear_ctx(ctx); - return -EINVAL; + return err; } /*
The support for NVME in-band authentication currently in the works ([1]) needs to generate ephemeral DH keys. Make dh-generic's ->set_secret() to generate an ephemeral key via the recently added crypto_dh_gen_privkey() in case the input ->key_size is zero. Note that this behaviour is in analogy to ecdh's ->set_secret(). [1] https://lkml.kernel.org/r/20211122074727.25988-4-hare@suse.de Signed-off-by: Nicolai Stange <nstange@suse.de> --- crypto/dh.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)