Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <CABvBcwY44BPa+TaDwxWaEogpg3Kdkq8o9cR5gSqNGF-o6d3jrw@mail.gmail.com>
 <20200716164028.1805047-1-lenaptr@google.com> <13569541.ZYm5mLc6kN@tauon.chronox.de>
In-Reply-To: <13569541.ZYm5mLc6kN@tauon.chronox.de>
From:   Elena Petrova <lenaptr@google.com>
Date:   Tue, 21 Jul 2020 13:55:14 +0100
Message-ID: <CABvBcwZ5mQPVFNpw=mY29cXo8oU8yviW5FZEFdKBNLvaudH6Ow@mail.gmail.com>
Subject: Re: [PATCH v2] crypto: af_alg - add extra parameters for DRBG interface
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

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