Re: [PATCH] crypto: gcm - fix cacheline sharing

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Iuliana Prodan <iuliana.prodan@nxp.com>,
	Eric Biggers <ebiggers@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Horia Geanta <horia.geanta@nxp.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	"open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
	<linux-crypto@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	dl-linux-imx <linux-imx@nxp.com>
Subject: Re: [PATCH] crypto: gcm - fix cacheline sharing
Date: Thu, 6 Jun 2019 08:53:10 +0200	[thread overview]
Message-ID: <CAKv+Gu-DokZ179_Gx8_20v_pQ3w_CARKdO0xdsO8CRZJG1uOqA@mail.gmail.com> (raw)
In-Reply-To: <20190606064603.lvde6dproqi3vwcq@gondor.apana.org.au>

On Thu, 6 Jun 2019 at 08:46, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Jun 06, 2019 at 08:42:46AM +0200, Ard Biesheuvel wrote:
> > On Thu, 6 Jun 2019 at 08:37, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > >
> > > On Thu, May 30, 2019 at 04:31:09PM +0200, Ard Biesheuvel wrote:
> > > >
> > > > This might work:
> > > >
> > > > diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> > > > index c0ece44f303b..3d313d2a279a 100644
> > > > --- a/drivers/crypto/caam/caamalg.c
> > > > +++ b/drivers/crypto/caam/caamalg.c
> > > > @@ -1661,7 +1661,8 @@ static int aead_decrypt(struct aead_request *req)
> > > >   * allocate and map the skcipher extended descriptor for skcipher
> > > >   */
> > > >  static struct skcipher_edesc *skcipher_edesc_alloc(struct
> > > > skcipher_request *req,
> > > > -                                                  int desc_bytes)
> > > > +                                                  int desc_bytes,
> > > > +                                                  u8 const *input_iv)
> > > >  {
> > > >         struct crypto_skcipher *skcipher = crypto_skcipher_reqtfm(req);
> > > >         struct caam_ctx *ctx = crypto_skcipher_ctx(skcipher);
> > > > @@ -1745,7 +1746,7 @@ static struct skcipher_edesc
> > > > *skcipher_edesc_alloc(struct skcipher_request *req,
> > > >         /* Make sure IV is located in a DMAable area */
> > > >         if (ivsize) {
> > > >                 iv = (u8 *)edesc->hw_desc + desc_bytes + sec4_sg_bytes;
> > > > -               memcpy(iv, req->iv, ivsize);
> > > > +               memcpy(iv, input_iv, ivsize);
> > > >
> > > >                 iv_dma = dma_map_single(jrdev, iv, ivsize, DMA_TO_DEVICE);
> > > >                 if (dma_mapping_error(jrdev, iv_dma)) {
> > >
> > > Hi Ard:
> > >
> > > I presume you will be submitting this patch at some point?  When
> > > you do please base it on top of your other one which I'm about to
> > > merge.
> >
> > I'm not sure I follow. Do you want a better fix for the CBC output IV
> > going forward? Or is this about other modes?
>
> You sent me a patch to fix CTR mode:
>
> https://patchwork.kernel.org/patch/10969747/
>

That patch does not fix CTR mode, it just disables copying the output
IV altogether for CTR mode so the DMA corruption does not occur.

> But your suggested fix for CBC mode itself where we need to do the
> copy (as seen quoted above) hasn't been submitted.
>

That same patch 'fixes' CBC, since CBC was never broken to begin with.
The CTS driver does not have something like the auth_tag sharing the
same cacheline with the IV, so CBC has always worked fine.

So I guess what you are after is a patch that, instead of dodging the
issue by limiting the copy to CBC, does not perform the copy at all
while anything is mapped for DMA? Then we can leave it up to the NXP
engineers to fix CTR mode.