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

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

On 2/12/2019 11:13 AM, Ard Biesheuvel wrote:
> On Fri, 8 Feb 2019 at 09:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On Fri, 8 Feb 2019 at 09:41, Horia Geanta <horia.geanta@nxp.com> wrote:
>>>
>>> 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)
>>
>> This violates the DMA api, since you are touching memory that is owned
>> by the device at this point (even though the addresses do not actually
>> overlap). Note that on architectures that support non-cache coherent
>> DMA, the kmalloc alignment is at least the cacheline size, for this
>> exact reason.
>>
> 
> Actually, the driver does violate the DMA api in another way:
> scatterwalk_map_and_copy() is accessing req->src after DMA mapping it.
> Does the issue still exist if scatterwalk_map_and_copy() is done
> before the DMA map?
> 
> (On a non-cache coherent system, the DMA map will typically perform a
> clean+invalidate, which means that the invalidate that occurs at unmap
> time cannot corrupt adjacent data, but this only works if the CPU does
> not write to the same cacheline while it is mapped for the device)
> 
scatterwalk_map_and_copy() is reading from req->src.
Are you saying it's forbidden for CPU to read from an area after it's DMA mapped?

Thanks,
Horia