Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

[Gilad Ben-Yossef]

On Tue, Sep 5, 2017 at 6:33 PM, Horia GeantÄ <horia.geanta@xxxxxxx> wrote:
> On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote:
>> Hi,
>>
>> On Thu, Jun 29, 2017 at 1:19 PM, Horia GeantÄ <horia.geanta@xxxxxxx> wrote:
>>> On 6/28/2017 4:42 PM, Horia GeantÄ wrote:
>>>> On 6/28/2017 4:27 PM, David Gstir wrote:
>>>>> Certain cipher modes like CTS expect the IV (req->info) of
>>>>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>>>>> contain the last ciphertext block when the {en,de}crypt operation is done.
>>>>> This is currently not the case for the CAAM driver which in turn breaks
>>>>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>>>>
>>>>> This patch fixes the CAAM driver to properly set the IV after the
>>>>> {en,de}crypt operation of ablkcipher finishes.
>>>>>
>>>>> This issue was revealed by the changes in the SW CTS mode in commit
>>>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>>>>>
>>>>> Cc: <stable@xxxxxxxxxxxxxxx> # 4.8+
>>>>> Signed-off-by: David Gstir <david@xxxxxxxxxxxxx>
>>>> Reviewed-by: Horia GeantÄ <horia.geanta@xxxxxxx>
>>>>
>>> Btw, instead of updating the IV in SW, CAAM engine could be programmed
>>> to do it - by saving the Context Register of the AES accelerator.
>>>
>>> Unfortunately this would require changes in quite a few places: shared
>>> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others.
>>>
>>> So it's better to have this fix now (which, considering size, is
>>> appropriate for -stable) and later, if needed, offload IV updating in HW.
>>>
>>
>> My apologies for reviving this thread from the dead, but doesn't the patch fail
>> for in-place decryption since we are copying from req->dst after
>> the operation is done, and therefore it no longer contains the ciphertext?
>>
> You are right, thanks! Will follow up with a fix.
> Though cts(cbc(aes)) in particular is working, see below.
>
>> I'm asking since I ran into a similar issue in the ccree driver and thought
>> to deploy a similar fix but could not convince myself why this works.
>>
> IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM
> engine) works since SW implementation of cts, when performing the
> ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but
> a previously value, saved before staring decryption in crypto_cts_decrypt():
>
> if (cbc_blocks <= 1)
>         memcpy(space, req->iv, bsize);
> else
>         scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize,
>                                  bsize, 0);
>

Is that not a performance bug in software CTS than? I mean all those
transformation
drivers doing that extra copy and possibly malloc and free to save the
data for the info
and than have the CTS implementation ignore that and do its own memory copy?

Gilad
-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru