Linux-Crypto Archive on lore.kernel.org
 help / color / Atom feed
From: Elena Petrova <lenaptr@google.com>
To: Stephan Mueller <smueller@chronox.de>
Cc: "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
	<linux-crypto@vger.kernel.org>,
	Eric Biggers <ebiggers@kernel.org>,
	Ard Biesheuvel <ardb@kernel.org>,
	Jeffrey Vander Stoep <jeffv@google.com>
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface
Date: Tue, 21 Jul 2020 13:55:14 +0100
Message-ID: <CABvBcwZ5mQPVFNpw=mY29cXo8oU8yviW5FZEFdKBNLvaudH6Ow@mail.gmail.com> (raw)
In-Reply-To: <13569541.ZYm5mLc6kN@tauon.chronox.de>

Hi Stephan,

On Mon, 20 Jul 2020 at 18:35, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Donnerstag, 16. Juli 2020, 18:40:28 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> sorry for the delay in answering.
>
> > Extending the userspace RNG interface:
> >   1. adding ALG_SET_DRBG_ENTROPY setsockopt option for entropy input;
> >   2. using sendmsg syscall for specifying the additional data.
> >
> > Signed-off-by: Elena Petrova <lenaptr@google.com>
> > ---
> >
> > libkcapi patch for testing:
> >
> > https://github.com/Len0k/libkcapi/commit/6f095d270b982008f419078614c15caa59
> > 2cb531
>
> If that kernel patch is taken, I would be happy to take the libkcapi patch
> with the following suggested changes:
>
> - add full documentation of kcapi_rng_set_entropy and kcapi_rng_send_addtl to
> kcapi.h
>
> - move test_cavp into either test/kcapi-main.c or its own application inside
> test/ where the caller provides the entropy_input, personalization string,
> additional inputs

Ok, thanks!

> >
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index 091c0a0bbf26..8484617596d1 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -1896,6 +1896,14 @@ config CRYPTO_STATS
> >  config CRYPTO_HASH_INFO
> >       bool
> >
> > +config CRYPTO_CAVS_DRBG
>
> CAVS is dead, long live ACVT :-) So, maybe use CAVP or ACVT as abbreviations?

Ok, let's use CAVP then.

> As the config option applies to AF_ALG, wouldn't it be better to use an
> appropriate name here like CRYPTO_USER_API_CAVP_DRBG or similar?
>
> Note, if indeed we would add akcipher or even KPP with CAVP support, maybe we
> need additional config options here. So, I would recommend to use this
> separate name space.

Yes, that makes sense, thanks.

> > +     tristate "Enable CAVS testing of DRBG"
>
> Dto: replace CAVS

Ack

> > +#ifdef CONFIG_CRYPTO_CAVS_DRBG
> > +static int rng_setentropy(void *private, const u8 *entropy, unsigned int
> > len) +{
> > +     struct rng_parent_ctx *pctx = private;
> > +     u8 *kentropy = NULL;
> > +
> > +     if (!capable(CAP_SYS_ADMIN))
> > +             return -EPERM;
> > +
> > +     if (pctx->entropy)
> > +             return -EINVAL;
> > +
> > +     if (len > MAXSIZE)
> > +             len = MAXSIZE;
> > +
> > +     if (len) {
> > +             kentropy = memdup_user(entropy, len);
> > +             if (IS_ERR(kentropy))
> > +                     return PTR_ERR(kentropy);
> > +     }
> > +
> > +     crypto_rng_alg(pctx->drng)->set_ent(pctx->drng, kentropy, len);
> > +     pctx->entropy = kentropy;
>
> Why do you need to keep kentropy around? For the check above whether entropy
> was set, wouldn't a boolean suffice?

I need to keep the pointer to free it after use. Unlike the setting of
the key, DRBG saves the entropy pointer in one of its internal
structures, but doesn't do any memory
management. I had only two ideas on how to prevent memory leaks:
either change drbg code to deal with the memory, or save the pointer
somewhere inside the socket. I opted for the latter. But if you know a
better approach I'm happy to rework my code accordingly.

> > diff --git a/include/crypto/if_alg.h b/include/crypto/if_alg.h
> > index 56527c85d122..312fdb3469cf 100644
> > --- a/include/crypto/if_alg.h
> > +++ b/include/crypto/if_alg.h
> > @@ -46,6 +46,7 @@ struct af_alg_type {
> >       void *(*bind)(const char *name, u32 type, u32 mask);
> >       void (*release)(void *private);
> >       int (*setkey)(void *private, const u8 *key, unsigned int keylen);
> > +     int (*setentropy)(void *private, const u8 *entropy, unsigned int len);
> >       int (*accept)(void *private, struct sock *sk);
> >       int (*accept_nokey)(void *private, struct sock *sk);
> >       int (*setauthsize)(void *private, unsigned int authsize);
> > @@ -123,7 +124,7 @@ struct af_alg_async_req {
> >   * @tsgl_list:               Link to TX SGL
> >   * @iv:                      IV for cipher operation
> >   * @aead_assoclen:   Length of AAD for AEAD cipher operations
> > - * @completion:              Work queue for synchronous operation
> > + * @wait:            Wait on completion of async crypto ops
>
> What is this change about? I am not sure it relates to the changes above.

I noticed that the documentation for the function differs from the
function declaration, so I thought I'd fix that, since I touched the
header. But yeah, it doesn't relate to the changes.

> Ciao
> Stephan

Thanks,
Elena

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-13 16:48 [PATCH 0/1] " Elena Petrova
2020-07-13 16:48 ` [PATCH 1/1] " Elena Petrova
2020-07-13 17:10   ` Eric Biggers
2020-07-16 14:23     ` Elena Petrova
2020-07-16 16:40       ` [PATCH v2] " Elena Petrova
2020-07-20 17:35         ` Stephan Mueller
2020-07-21 12:55           ` Elena Petrova [this message]
2020-07-21 13:18             ` Stephan Mueller
2020-07-28 16:16               ` Elena Petrova
2020-07-20 17:42         ` Stephan Müller
2020-07-22 15:59         ` Eric Biggers
2020-07-28 15:51           ` [PATCH v3] " Elena Petrova
2020-07-28 17:36             ` Eric Biggers
2020-07-29 15:45               ` [PATCH v4] " Elena Petrova
2020-07-29 19:26                 ` Stephan Müller
2020-07-31  7:23                 ` Herbert Xu
2020-08-03 14:48                   ` Elena Petrova
2020-08-03 15:10                     ` Stephan Mueller
2020-08-03 15:30                       ` Elena Petrova
2020-08-04  2:18                     ` Herbert Xu
2020-07-13 17:25   ` [PATCH 1/1] " Eric Biggers
2020-07-31  7:26     ` Herbert Xu
2020-08-13 16:00       ` Elena Petrova
2020-08-13 16:01         ` [PATCH v4] " Elena Petrova
2020-08-13 16:04           ` Elena Petrova
2020-08-13 16:08             ` [PATCH v5] " Elena Petrova
2020-08-13 19:32               ` Eric Biggers
2020-08-21  4:24                 ` Herbert Xu
2020-09-08 17:04                   ` [PATCH v6] " Elena Petrova
2020-09-09  4:35                     ` Eric Biggers
2020-09-09 18:29                       ` [PATCH v7] " Elena Petrova
2020-09-09 21:00                         ` Eric Biggers
2020-09-16 11:07                           ` [PATCH v8] " Elena Petrova
2020-09-18  6:43                             ` Herbert Xu
2020-09-18 15:42                               ` [PATCH v9] " Elena Petrova
2020-09-08 17:23                   ` [PATCH v5] " Elena Petrova
2020-09-08 17:18                 ` Elena Petrova
2020-07-14  5:17 ` [PATCH 0/1] " Stephan Mueller
2020-07-14 15:23   ` Elena Petrova
2020-07-14 15:34     ` Stephan Mueller
2020-07-16 14:41       ` Elena Petrova
2020-07-16 14:49         ` Stephan Mueller
2020-07-16 14:59           ` Stephan Mueller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABvBcwZ5mQPVFNpw=mY29cXo8oU8yviW5FZEFdKBNLvaudH6Ow@mail.gmail.com' \
    --to=lenaptr@google.com \
    --cc=ardb@kernel.org \
    --cc=ebiggers@kernel.org \
    --cc=jeffv@google.com \
    --cc=linux-crypto@vger.kernel.org \
    --cc=smueller@chronox.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Crypto Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-crypto/0 linux-crypto/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-crypto linux-crypto/ https://lore.kernel.org/linux-crypto \
		linux-crypto@vger.kernel.org
	public-inbox-index linux-crypto

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-crypto


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git