linux-kernel - Re: [BUG] crypto: ccree: driver does not handle case where cryptlen = authsize =0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Feb 2022 11:11:43 +0200
From:   Gilad Ben-Yossef <gilad@...yossef.com>
To:     Corentin Labbe <clabbe.montjoie@...il.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@...r.kernel.org>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [BUG] crypto: ccree: driver does not handle case where cryptlen =
 authsize =0

Hi,

On Tue, Feb 22, 2022 at 9:39 AM Gilad Ben-Yossef <gilad@...yossef.com> wrote:
>
> On Mon, Feb 21, 2022 at 4:05 PM Corentin Labbe
> <clabbe.montjoie@...il.com> wrote:
> >
> > Le Mon, Feb 21, 2022 at 12:08:12PM +0200, Gilad Ben-Yossef a écrit :
> > > Hi,
> > >
> > > On Sun, Feb 20, 2022 at 9:26 PM Corentin Labbe
> > > <clabbe.montjoie@...il.com> wrote:
> > > >
> > > ...
> > > >
> > > > Hello
> > > >
> > > > While testing your patch for this problem, I saw another warning (unrelated with your patch):
> > >
> > > Dear Corentin, you are a treasure trove of bug reports. I love it.
> > > Thank you! :-)
> > >
> > > > [   34.061953] ------------[ cut here ]------------
> ...
> > >
> > > So, this is an interesting one.
> > > What I *think* is happening is that the drbg implementation is
> > > actually doing something naughty: it is passing the same exact memory
> > > buffer, both as source and destination to an encryption operation to
> > > the crypto skcipher API, BUT via two different scatter gather lists.
> > >
> > > I'm not sure but I believe this is not a legitimate use of the API,
> > > but before we even go into this, let's see if this little fix helps at
> > > all and this is indeed the root cause.
> > >
> > > Can you test this small change for me, please?
> > >
> > > diff --git a/crypto/drbg.c b/crypto/drbg.c
> > > index 177983b6ae38..13824fd27627 100644
> > > --- a/crypto/drbg.c
> > > +++ b/crypto/drbg.c
> > > @@ -1851,7 +1851,7 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
> > >                 /* Use scratchpad for in-place operation */
> > >                 inlen = scratchpad_use;
> > >                 memset(drbg->outscratchpad, 0, scratchpad_use);
> > > -               sg_set_buf(sg_in, drbg->outscratchpad, scratchpad_use);
> > > +               sg_in = sg_out;
> > >         }
> > >
> > >         while (outlen) {
> > >
> >
> > No more stacktrace !
>
> Thank you. I will send a patch later today.

> --
> Gilad Ben-Yossef
> Chief Coffee Drinker
>
> values of β will give rise to dom!

OK, it seems my direction of fixing the caller site has not been taken
kindly by the power that be.
Let's try something else.

Can you please drop the previous patch and test this one instead?

diff --git a/drivers/crypto/ccree/cc_buffer_mgr.c
b/drivers/crypto/ccree/cc_buffer_mgr.c
index 11e0278c8631..398843040566 100644
--- a/drivers/crypto/ccree/cc_buffer_mgr.c
+++ b/drivers/crypto/ccree/cc_buffer_mgr.c
@@ -377,6 +377,7 @@ int cc_map_cipher_request(struct cc_drvdata
*drvdata, void *ctx,
        u32 dummy = 0;
        int rc = 0;
        u32 mapped_nents = 0;
+       int src_direction = (src != dst ? DMA_TO_DEVICE : DMA_BIDIRECTIONAL);

        req_ctx->dma_buf_type = CC_DMA_BUF_DLLI;
        mlli_params->curr_pool = NULL;
@@ -399,7 +400,7 @@ int cc_map_cipher_request(struct cc_drvdata
*drvdata, void *ctx,
        }

        /* Map the src SGL */
-       rc = cc_map_sg(dev, src, nbytes, DMA_BIDIRECTIONAL, &req_ctx->in_nents,
+       rc = cc_map_sg(dev, src, nbytes, src_direction, &req_ctx->in_nents,
                       LLI_MAX_NUM_OF_DATA_ENTRIES, &dummy, &mapped_nents);
        if (rc)
                goto cipher_exit;
@@ -416,7 +417,7 @@ int cc_map_cipher_request(struct cc_drvdata
*drvdata, void *ctx,
                }
        } else {
                /* Map the dst sg */
-               rc = cc_map_sg(dev, dst, nbytes, DMA_BIDIRECTIONAL,
+               rc = cc_map_sg(dev, dst, nbytes, DMA_FROM_DEVICE,
                               &req_ctx->out_nents, LLI_MAX_NUM_OF_DATA_ENTRIES,
                               &dummy, &mapped_nents);
                if (rc)


Thanks!
Gilad

-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!