Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Iuliana Prodan <iuliana.prodan@nxp.com>
To:     Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Eric Biggers <ebiggers@kernel.org>
CC:     Herbert Xu <herbert@gondor.apana.org.au>,
        "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
Thread-Topic: [PATCH] crypto: gcm - fix cacheline sharing
Thread-Index: AQHVFkF/PjPULMOhfkiEXDJZ6fiBzQ==
Date:   Thu, 30 May 2019 13:29:41 +0000
Message-ID: <VI1PR04MB44459EEF7BCD3458BB3D143D8C180@VI1PR04MB4445.eurprd04.prod.outlook.com>
References: <1559149856-7938-1-git-send-email-iuliana.prodan@nxp.com>
 <20190529202728.GA35103@gmail.com>
 <CAKv+Gu-4KqcY=WhwY98JigTzeXaL5ggYEcu7+kNzNtpO2FLQXg@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: b1320404-74f1-498a-d8a4-08d6e502df39
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 May 2019 13:29:41.6339
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: iuliana.prodan@nxp.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB4815
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 5/30/2019 1:16 AM, Ard Biesheuvel wrote:=0A=
> On Wed, 29 May 2019 at 22:27, Eric Biggers <ebiggers@kernel.org> wrote:=
=0A=
>>=0A=
>> On Wed, May 29, 2019 at 08:10:56PM +0300, Iuliana Prodan wrote:=0A=
>>> The generic GCM driver should ensure that whatever it passes into=0A=
>>> scatterlists is safe for non-cache coherent DMA.=0A=
>>> The issue was seen while running GCM on CAAM driver. But, since CAAM=0A=
>>> does not support GHASH on i.MX6, only CTR skcipher part of the GCM is=
=0A=
>>> offloaded.=0A=
>>> The skcipher request received by CAAM has req->src pointing to=0A=
>>> auth_tag[16] and req->iv pointing to iv[16]. Problem is that when=0A=
>>> the iv is updated (crypto API requires skcipher implementations to=0A=
>>> update the IV with the last ciphertext block) is written in iv[16],=0A=
>>> which is on the same cacheline as auth_tag[16] that was previously=0A=
>>> DMA mapped.=0A=
>>> Solution is to use a pointer, aligned to cache line, instead of auth_ta=
g=0A=
>>> buffer, for encryption/decryption and then free it on completion.=0A=
>>>=0A=
>>> Link: https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F=
%2Flore.kernel.org%2Flinux-crypto%2F20190208114459.5nixe76xmmkhur75%40gondo=
r.apana.org.au%2F&amp;data=3D02%7C01%7Ciuliana.prodan%40nxp.com%7C8426e8db8=
5774c4e829308d6e4834cf6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636947=
649914200400&amp;sdata=3DFq%2F83lofKoEnz7HnzY1jjU1jUi0b2QmVZPfZrzWXtrk%3D&a=
mp;reserved=3D0=0A=
>>> Cc: <stable@vger.kernel.org> # v4.19+=0A=
>>> Fixes: adcbc688fe2f ("crypto: gcm - Convert to new AEAD interface")=0A=
>>> Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>=0A=
>>> Signed-off-by: Iuliana Prodan <iuliana.prodan@nxp.com>=0A=
>>>=0A=
> ...=0A=
>> So what about the other places that also pass an IV located next to the =
data,=0A=
>> like crypto/ccm.c and crypto/adiantum.c?  If we're actually going to mak=
e this a=0A=
>> new API requirement, then we need to add a debugging option that makes t=
he API=0A=
>> detect this violation so that the other places can be fixed too.=0A=
>>=0A=
>> Also, doing a kmalloc() per requset is inefficient and very error-prone.=
  In=0A=
>> fact there are at least 3 bugs here: (1) not checking the return value, =
(2)=0A=
>> incorrectly using GFP_KERNEL when it may be atomic context, and (3) not =
always=0A=
>> freeing the memory.  Why not use cacheline-aligned memory within the req=
uest=0A=
>> context, so that a separate kmalloc() isn't needed?=0A=
>>=0A=
>> Also, did you consider whether there's any way to make the crypto API ha=
ndle=0A=
>> this automatically, so that all the individual users don't have to?=0A=
>>=0A=
> =0A=
> Reading back that old thread, it appears that the core issue is that=0A=
> the IV is copied when the scatterlist is already mapped for DMA. This=0A=
> means the cacheline covering the IV and the auth tag is dirty while=0A=
> the non-coherent DMA transaction takes place, and given that we clean=0A=
> rather than invalidate the start and end of DMA mappings if they are=0A=
> not aligned to the cache writeback granule size, whatever sits in the=0A=
> cacheline overwrites whatever the device wrote in there.=0A=
> =0A=
> Iuliana, did you try pulling the IV copy forward? I.e.,=0A=
=0A=
I've tried coping the IV before the extended descriptor allocation, but =0A=
is not working and to make it work will need to make more changes in =0A=
CAAM. We need the original iv, and if we move it before =0A=
skcipher_edesc_alloc we lose it.=0A=
The fix exclusively in CAAM drv, to copy iv before DMA map, is more complex=
.=0A=
=0A=
> =0A=
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.=
c=0A=
> index c0ece44f303b..11e91c0c9a96 100644=0A=
> --- a/drivers/crypto/caam/caamalg.c=0A=
> +++ b/drivers/crypto/caam/caamalg.c=0A=
> @@ -1835,11 +1835,6 @@ static int skcipher_decrypt(struct skcipher_reques=
t *req)=0A=
>          u32 *desc;=0A=
>          int ret =3D 0;=0A=
> =0A=
> -       /* allocate extended descriptor */=0A=
> -       edesc =3D skcipher_edesc_alloc(req, DESC_JOB_IO_LEN * CAAM_CMD_SZ=
);=0A=
> -       if (IS_ERR(edesc))=0A=
> -               return PTR_ERR(edesc);=0A=
> -=0A=
>          /*=0A=
>           * The crypto API expects us to set the IV (req->iv) to the last=
=0A=
>           * ciphertext block.=0A=
> @@ -1848,6 +1843,11 @@ static int skcipher_decrypt(struct skcipher_reques=
t *req)=0A=
>                  scatterwalk_map_and_copy(req->iv, req->src, req->cryptle=
n -=0A=
>                                           ivsize, ivsize, 0);=0A=
> =0A=
> +       /* allocate extended descriptor */=0A=
> +       edesc =3D skcipher_edesc_alloc(req, DESC_JOB_IO_LEN * CAAM_CMD_SZ=
);=0A=
> +       if (IS_ERR(edesc))=0A=
> +               return PTR_ERR(edesc);=0A=
> +=0A=
>          /* Create and submit job descriptor*/=0A=
>          init_skcipher_job(req, edesc, false);=0A=
>          desc =3D edesc->hw_desc;=0A=
> =0A=
> This should ensure that the cacheline is cleaned when the DMA mapping=0A=
> is created.=0A=
> =0A=
=0A=
Thanks,=0A=
Iulia=0A=