On 2/5/2019 10:31 AM, Sascha Hauer wrote:
> Hi Horia,
>
> On Mon, Feb 04, 2019 at 12:26:26PM +0000, Horia Geanta wrote:
>> On 1/31/2019 8:12 AM, Sascha Hauer wrote:
>>> In skcipher_decrypt() the IV passed in by the caller is overwritten and
>>> the tcrypt module fails with:
>>>
>>> alg: aead: decryption failed on test 1 for gcm_base(ctr-aes-caam,ghash-generic): ret=74
>>> alg: aead: Failed to load transform for gcm(aes): -2
>>>
>>> With this patch tcrypt runs without errors.
>>>
>> This doesn't mean the patch is correct.
>> crypto API requires skcipher implementations to update the IV with the last
>> ciphertext block.
>>
>> 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.
>
> I can confirm that this solves it here on my side aswell.
>
Thanks for confirming.
> I have no idea what's best to do here, but I'd like to have that fixed.
>
Yes, me too.
I would wait for Herbert's input though. As I said, it's a bit weird for a
skcipher implementation to be aware of updating req->iv having such a side
effect on req->src.
> Is there some easy to reproduce testcase to show the issues that arise
> with my patch? Apparently tcrypt doesn't do chaining, right?
>
In theory cts(cbc(aes)) decryption would have to fail when cbc(aes) would
execute on caam and cts() in SW.
However, cts() SW implementation (crypto/cts.c) does not rely on underlying
cbc(aes) to update the IV, instead it uses a temporary saved IV - see discussion
here:
https://www.mail-archive.com/linux-crypto@xxxxxxxxxxxxxxx/msg27719.html
Horia
