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>
 <13569541.ZYm5mLc6kN@tauon.chronox.de> <CABvBcwZ5mQPVFNpw=mY29cXo8oU8yviW5FZEFdKBNLvaudH6Ow@mail.gmail.com>
 <9149882.4vTCxPXJkl@tauon.chronox.de>
In-Reply-To: <9149882.4vTCxPXJkl@tauon.chronox.de>
From:   Elena Petrova <lenaptr@google.com>
Date:   Tue, 28 Jul 2020 17:16:16 +0100
Message-ID: <CABvBcwbDLsy+bQxsFwhjcZRkHz1jbP6VC7Mx8tgLMoUbmYBE_A@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 Tue, 21 Jul 2020 at 14:19, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Dienstag, 21. Juli 2020, 14:55:14 CEST schrieb Elena Petrova:
>
> Hi Elena,
>
> > > > +#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.
>
> I was thinking of calling crypto_rng_alg(pctx->drng)->seed() directly after
> set_ent. This call performs a DRBG instantatiate where the entropy buffer is
> used. See crypto_drbg_reset_test for the approach.
>
> But maybe you are right, the test "entropy" buffer inside the DRBG currently
> cannot be reset. So, for sanity purposes, you need to keep it around.

I looked into this, and afaik `->seed()` needs the seed buffer (a.k.a.
key); and seed() is also invoked on ALG_SET_KEY setsockopt. So we
would need both entropy and seed values at the same time. To avoid
complicating the matters, I decided to leave the code as is. I added a
comment in v3 [https://lore.kernel.org/linux-crypto/20200728155159.2156480-1-lenaptr@google.com/]
explaining why the `kentropy` pointer is saved.

> Ciao
> Stephan

Regards,
Elena