> On 17 Feb 2017, at 10:06, Dennis Chen <dennis.chen@xxxxxxx> wrote:
>
>> On Fri, Feb 17, 2017 at 09:23:00AM +0000, Ard Biesheuvel wrote:
>>
>>> On 17 Feb 2017, at 09:17, Dennis Chen <dennis.chen@xxxxxxx> wrote:
>>>
>>> Hello Ard,
>>> Morning!
>>>> On Fri, Feb 17, 2017 at 07:12:46AM +0000, Ard Biesheuvel wrote:
>>>> Hello Libo,
>>>>
>>>>> On 17 February 2017 at 03:47, <Libo.Wang@xxxxxxx> wrote:
>>>>> From: Libo Wang <Libo Wang@xxxxxxx>
>>>>>
>>>>> CTS template assumes underlying CBC algorithm will carry out next IV for
>>>>> further process.But some implementations of CBC algorithm in kernel break
>>>>> this assumption, for example, some hardware crypto drivers ignore next IV
>>>>> for performance consider, inthis case, tcry cts(cbc(aes)) test case will
>>>>> fail. This patch is trying to fix it by getting next IV information ready
>>>>> before last two blocks processed.
>>>>>
>>>>> Signed-off-by: Libo Wang <libo.wang@xxxxxxx>
>>>>> Signed-off-by: Dennis Chen <dennis.chen@xxxxxxx>
>>>>
>>>> Which algorithms in particular break this assumption? I recently fixed
>>>> some ARM accelerated software drivers for this reason. If there are
>>>> others, we should fix those rather than try to fix it in the CTS
>>>> driver.
>>>>
>>> There're some ARM HW accelerated drivers which are not upstream yet(IP license)
>>> breaks this assumption. The current CTS template requires the underlying CBC
>>> algorithm always fill the next IV, but in some cases like CBC standalone mode,
>>> it doesn't need to fill the next IV, so apparently it will induce performance
>>> degrade from the point of drivers, that's why we fix it in the CTS template,except
>>> that it will also mitigate the potential defect of other HW-based CBC drivers.
>>
>> You should fix this in the driver, not in the cts code.
>>
> Ah, we're open for the change, but is it better to give the reason to fix it in the driver
> instead of in cts code?
Because it is the h/w driver that violates the api.
Please search the list: I discussed this with Herbert very recently
>>>>
>>>>> ---
>>>>> crypto/cts.c | 29 +++++++++++++++++++++++++----
>>>>> 1 file changed, 25 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/crypto/cts.c b/crypto/cts.c
>>>>> index a1335d6..712164b 100644
>>>>> --- a/crypto/cts.c
>>>>> +++ b/crypto/cts.c
>>>>> @@ -154,6 +154,7 @@ static int crypto_cts_encrypt(struct skcipher_request *req)
>>>>> unsigned int nbytes = req->cryptlen;
>>>>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1;
>>>>> unsigned int offset;
>>>>> + int ret = 0;
>>>>>
>>>>> skcipher_request_set_tfm(subreq, ctx->child);
>>>>>
>>>>> @@ -174,8 +175,17 @@ static int crypto_cts_encrypt(struct skcipher_request *req)
>>>>> skcipher_request_set_crypt(subreq, req->src, req->dst,
>>>>> offset, req->iv);
>>>>>
>>>>> - return crypto_skcipher_encrypt(subreq) ?:
>>>>> - cts_cbc_encrypt(req);
>>>>> + /* process CBC blocks */
>>>>> + ret = crypto_skcipher_encrypt(subreq);
>>>>> + /* process last two blocks */
>>>>> + if (!ret) {
>>>>
>>>> What happens if an async driver returns -EINPROGRESS here?
>>> For this case, the CTS will return -EINPROGRESS directly.
>>>
>>
>> I see. But how do you handle the missing out IV when the async call completes?
>>
> Not quite understand here, is there some logic inconsistent of the change here comparing the
> original one to handle this case? We think it will have the same result if the fix is in driver layer.
>
> Thanks,
> Dennis
>>
>>>>
>>>>> + /* Get IVn-1 back */
>>>>> + scatterwalk_map_and_copy(req->iv, req->dst, (offset - bsize), bsize, 0);
>>>>> + /* Continue last two blocks */
>>>>> + return cts_cbc_encrypt(req);
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> static int cts_cbc_decrypt(struct skcipher_request *req)
>>>>> @@ -248,6 +258,8 @@ static int crypto_cts_decrypt(struct skcipher_request *req)
>>>>> int cbc_blocks = (nbytes + bsize - 1) / bsize - 1;
>>>>> unsigned int offset;
>>>>> u8 *space;
>>>>> + int ret = 0;
>>>>> + u8 iv_next[bsize];
>>>>>
>>>>> skcipher_request_set_tfm(subreq, ctx->child);
>>>>>
>>>>> @@ -277,8 +289,17 @@ static int crypto_cts_decrypt(struct skcipher_request *req)
>>>>> skcipher_request_set_crypt(subreq, req->src, req->dst,
>>>>> offset, req->iv);
>>>>>
>>>>> - return crypto_skcipher_decrypt(subreq) ?:
>>>>> - cts_cbc_decrypt(req);
>>>>> + /* process last two blocks */
>>>>> + scatterwalk_map_and_copy(iv_next, req->src, (offset - bsize), bsize, 0);
>>>>> + ret = crypto_skcipher_decrypt(subreq);
>>>>> + if (!ret) {
>>>>> + /* Set Next IV */
>>>>> + subreq->iv = iv_next;
>>>>> + /* process last two blocks */
>>>>> + return cts_cbc_decrypt(req);
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> static int crypto_cts_init_tfm(struct crypto_skcipher *tfm)
>>>>> --
>>>>> 1.8.3.1
>>>>>
