Re: [PATCH] crypto: caam - Do not overwrite IV

[Horia Geanta <horia.geanta@nxp.com>]

On 2/8/2019 9:16 AM, Herbert Xu wrote:
> On Mon, Feb 04, 2019 at 12:26:26PM +0000, Horia Geanta wrote:
>>
>> The root cause of the issue is cache line sharing.
>>
>> struct crypto_gcm_req_priv_ctx {
>>         u8 iv[16];
>>         u8 auth_tag[16];
>> 	[...]
>> };
>>
>> Since caam does not support ghash on i.MX6, only ctr skcipher part of the gcm is
>> offloaded.
>> The skcipher request received by caam has req->src pointing to auth_tag[16] (1st
>> S/G entry) and req->iv pointing to iv[16].
>> caam driver:
>> 1-DMA maps req->src
>> 2-copies original req->iv to internal buffer
>> 3-updates req->iv (scatterwalk_map_and_copy from last block in req->src)
>> 4-sends job to crypto engine
>>
>> Problem is that operation 3 above is writing iv[16], which is on the same cache
>> line as auth_tag[16] that was previously DMA mapped.
>>
>> I've checked that forcing auth_tag and iv to be on separate cache lines
>> -       u8 auth_tag[16];
>> +       u8 auth_tag[16] ____cacheline_aligned;
>> solves the issue.
>>
>> OTOH, maybe the fix should be done in caam driver, by avoiding any writes
>> (touching any data, even seemingly unrelated req->iv) after DMA mapping
>> req->src, req->dst etc.
>> Having req->iv and req->src sharing the same cache line is unfortunate.
>>
>> Herbert, what do you think?
> 
> Well just like the other cases if your input is a kernel pointer you
> must not perform DMA on it.  Only SG lists can be used for DMA.
> 
As I said at point 2 above, req->iv is copied to an internal buffer, which is
allocated using kmalloc.

> So the IV needs to be copied on completion.
> 
Is it mandatory to be copied *on completion*?
In some cases implementations could update req->iv before completion - for e.g.
in case of cbc decryption the last block from req->src is copied into req->iv
before the engine performs decryption (to avoid in-place decryption, where the
last block would be overwritten).

I'll try to explain issue at hand in more detail.

------------------
|       IV       |
------------------
|  input buffer  |
------------------

Consider that the skcipher implementation receives, via crypto API, a request
with req->IV pointing to "IV" and, for simplicity, a 1-entry req->src
scatterlist pointing at "input buffer".

In caam's particular case (and for decryption):
a-req->src is DMA mapped
b-req->iv is overwritten with last block of req->src
c-crypto engine executes decryption (using the original value of req->iv)

If IV and input buffer are on the same cache line, there is a problem when the
device is non-coherent (i.MX case) since CPU is touching part of the cache line
(writing the IV) after DMA API mapping was called for the same cacheline
(req->src -> input buffer).

I don't think we could ask an implementation to be aware of the memory layout of
req->iv and req->src (and req->dst etc.) buffers.

If I am not mistaken, req->src for skcipher request in crypto/gcm.c is breaking
one of the DMA API rules - Documentation/DMA-API.txt:

.. warning::

        Memory coherency operates at a granularity called the cache
        line width.  In order for memory mapped by this API to operate
        correctly, the mapped region must begin exactly on a cache line
        boundary and end exactly on one (to prevent two separately mapped
        regions from sharing a single cache line).

So if there is a real intention to support offloading skcipher, as this old
commit suggests:

84c911523020 ("[CRYPTO] gcm: Add support for async ciphers")
    This patch adds the necessary changes for GCM to be used with async
    ciphers.  This would allow it to be used with hardware devices that
    support CTR.

then we must take special care when building skcipher req->src and avoid having
it's first entry (auth_tag[16] in crypto_gcm_req_priv_ctx structure) sharing a
cache line with req->iv.

Thanks,
Horia