mbox series

[v2,0/4] Additional fixes on Talitos driver

Message ID cover.1560263641.git.christophe.leroy@c-s.fr (mailing list archive)
Headers show
Series Additional fixes on Talitos driver | expand

Message

Christophe Leroy June 11, 2019, 2:39 p.m. UTC 
This series is the last set of fixes for the Talitos driver.

We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:

[    3.385197] bus: 'platform': really_probe: probing driver talitos with device ff020000.crypto
[    3.450982] random: fast init done
[   12.252548] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[   12.262226] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[   43.310737] Bug in SEC1, padding ourself
[   45.603318] random: crng init done
[   54.612333] talitos ff020000.crypto: fsl,sec1.2 algorithms registered in /proc/crypto
[   54.620232] driver: 'talitos': driver_bound: bound to device 'ff020000.crypto'

[    1.193721] bus: 'platform': really_probe: probing driver talitos with device b0030000.crypto
[    1.229197] random: fast init done
[    2.714920] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[    2.724312] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos-hsna)
[    4.482045] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos)
[    4.490940] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[    4.500280] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos)
[    4.509727] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[    6.631781] random: crng init done
[   11.521795] talitos b0030000.crypto: fsl,sec2.2 algorithms registered in /proc/crypto
[   11.529803] driver: 'talitos': driver_bound: bound to device 'b0030000.crypto'

v2: dropped patch 1 which was irrelevant due to a rebase weirdness. Added Cc to stable on the 2 first patches.

Christophe Leroy (4):
  crypto: talitos - move struct talitos_edesc into talitos.h
  crypto: talitos - fix hash on SEC1.
  crypto: talitos - eliminate unneeded 'done' functions at build time
  crypto: talitos - drop icv_ool

 drivers/crypto/talitos.c | 98 ++++++++++++++++++++----------------------------
 drivers/crypto/talitos.h | 28 ++++++++++++++
 2 files changed, 69 insertions(+), 57 deletions(-)

Comments

Horia Geanta June 11, 2019, 3:37 p.m. UTC | #1 
On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> This series is the last set of fixes for the Talitos driver.
> 
> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
> 
I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
hmac(sha512):

alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[<reimport>76.49%@+4002, <reimport>23.51%@alignmask+26] iv_offset=4"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9"

Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern:

1.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[<reimport,nosimd>100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]"

2.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[<reimport>100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]"

3.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[<reimport>35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59"

If you run several times with fuzz testing enabled on your sec2.2,
are you able to see similar failures?

Thanks,
Horia
Christophe Leroy June 11, 2019, 3:40 p.m. UTC | #2 
Le 11/06/2019 à 17:37, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> This series is the last set of fixes for the Talitos driver.
>>
>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>
> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
> hmac(sha512):

Is that new with this series or did you already have it before ?

What do you mean by "fuzz testing" enabled ? Is that 
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?

Christophe

> 
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[<reimport>76.49%@+4002, <reimport>23.51%@alignmask+26] iv_offset=4"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9"
> 
> Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern:
> 
> 1.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[<reimport,nosimd>100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]"
> 
> 2.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[<reimport>100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]"
> 
> 3.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[<reimport>35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59"
> 
> If you run several times with fuzz testing enabled on your sec2.2,
> are you able to see similar failures?
> 
> Thanks,
> Horia
>
Horia Geanta June 11, 2019, 4:30 p.m. UTC | #3 
On 6/11/2019 6:40 PM, Christophe Leroy wrote:
> 
> 
> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> This series is the last set of fixes for the Talitos driver.
>>>
>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>
>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>> hmac(sha512):
> 
> Is that new with this series or did you already have it before ?
> 
Looks like this happens with or without this series.

I haven't checked the state of this driver for quite some time.
Since I've noticed increased activity, I thought it would be worth
actually testing the changes.

Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
strictly for sec 1.x or they affect all revisions?

> What do you mean by "fuzz testing" enabled ? Is that 
> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
> 
Yes, it's this config symbol.

Horia
Christophe Leroy June 11, 2019, 5:16 p.m. UTC | #4 
Le 11/06/2019 à 18:30, Horia Geanta a écrit :
> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> This series is the last set of fixes for the Talitos driver.
>>>>
>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>
>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>> hmac(sha512):
>>
>> Is that new with this series or did you already have it before ?
>>
> Looks like this happens with or without this series.
> 
> I haven't checked the state of this driver for quite some time.
> Since I've noticed increased activity, I thought it would be worth
> actually testing the changes.
> 
> Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
> strictly for sec 1.x or they affect all revisions?

They are strictly for sec 1.x

> 
>> What do you mean by "fuzz testing" enabled ? Is that
>> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>>
> Yes, it's this config symbol.

Indeed SEC 2.2 only supports up to SHA-256.

Christophe

> 
> Horia
>
Christophe Leroy June 12, 2019, 5:52 a.m. UTC | #5 
Le 11/06/2019 à 18:30, Horia Geanta a écrit :
> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> This series is the last set of fixes for the Talitos driver.
>>>>
>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>
>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>> hmac(sha512):
>>
>> Is that new with this series or did you already have it before ?
>>
> Looks like this happens with or without this series.

Found the issue, that's in 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad

CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.

Just sent a patch to fix it.

Thanks
Christophe

> 
> I haven't checked the state of this driver for quite some time.
> Since I've noticed increased activity, I thought it would be worth
> actually testing the changes.
> 
> Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
> strictly for sec 1.x or they affect all revisions?
> 
>> What do you mean by "fuzz testing" enabled ? Is that
>> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>>
> Yes, it's this config symbol.
> 
> Horia
>
Horia Geanta June 12, 2019, 1:59 p.m. UTC | #6 
On 6/12/2019 8:52 AM, Christophe Leroy wrote:
> 
> 
> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>
>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>
>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>> hmac(sha512):
>>>
>>> Is that new with this series or did you already have it before ?
>>>
>> Looks like this happens with or without this series.
> 
> Found the issue, that's in 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
> 
> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
> 
> Just sent a patch to fix it.
> 
Thanks, I've tested it and the hmac failures go away.

However, testing gets stuck.
Seems there is another issue lurking in the driver.

Used cryptodev-2.6/master with the following on top:
crypto: testmgr - add some more preemption points
	https://patchwork.kernel.org/patch/10972337/
crypto: talitos - fix max key size for sha384 and sha512
	https://patchwork.kernel.org/patch/10988473/

[...]
alg: skcipher: skipping comparison tests for ecb-3des-talitos because ecb(des3_ede-generic) is unavailable
INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
      Not tainted 5.2.0-rc1-g905bfd415e8a #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
cryptomgr_test  D    0   314      2 0x00000800
Call Trace:
[e78337e0] [00000004] 0x4 (unreliable)
[e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
[e78338f8] [c08a7158] schedule+0x34/0xc8
[e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
[e7833958] [c08a7be4] wait_for_common+0xa0/0x164
[e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
[e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
[e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
[e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
[e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
[e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
[e7833f08] [c006c410] kthread+0xf8/0x124
[e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c

addr2line on c03aba00 points to crypto/testmgr.c:1335

   1327)  if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
   1328)      vec->digest_error) {
   1329)          /* Just using digest() */
   1330)          ahash_request_set_callback(req, req_flags, crypto_req_done,
   1331)                                     &wait);
   1332)          ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
   1333)          err = do_ahash_op(crypto_ahash_digest, req, &wait, cfg->nosimd);
   1334)          if (err) {
-> 1335)                  if (err == vec->digest_error)
   1336)                          return 0;
   1337)                  pr_err("alg: ahash: %s digest() failed on test vector %s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
   1338)                         driver, vec_name, vec->digest_error, err,
   1339)                         cfg->name);
   1340)                  return err;
   1341)          }
   1342)          if (vec->digest_error) {
   1343)                  pr_err("alg: ahash: %s digest() unexpectedly succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
   1344)                         driver, vec_name, vec->digest_error, cfg->name);
   1345)                  return -EINVAL;
   1346)          }
   1347)          goto result_ready;
   1348)  }

Seems that for some reason driver does not receive the interrupt from HW,
thus completion callback does not run.

Tried with or without current patch series, no change in behaviour.

If you cannot reproduce and don't have any idea, I'll try the hard way
(git bisect).

Thanks,
Horia
Christophe Leroy June 13, 2019, 4:57 a.m. UTC | #7 
Le 12/06/2019 à 15:59, Horia Geanta a écrit :
> On 6/12/2019 8:52 AM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>>
>>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>>
>>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>>> hmac(sha512):
>>>>
>>>> Is that new with this series or did you already have it before ?
>>>>
>>> Looks like this happens with or without this series.
>>
>> Found the issue, that's in
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
>>
>> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
>>
>> Just sent a patch to fix it.
>>
> Thanks, I've tested it and the hmac failures go away.
> 
> However, testing gets stuck.
> Seems there is another issue lurking in the driver.
> 
> Used cryptodev-2.6/master with the following on top:
> crypto: testmgr - add some more preemption points
> 	https://patchwork.kernel.org/patch/10972337/
> crypto: talitos - fix max key size for sha384 and sha512
> 	https://patchwork.kernel.org/patch/10988473/
> 
> [...]
> alg: skcipher: skipping comparison tests for ecb-3des-talitos because ecb(des3_ede-generic) is unavailable
> INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
>        Not tainted 5.2.0-rc1-g905bfd415e8a #1
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> cryptomgr_test  D    0   314      2 0x00000800
> Call Trace:
> [e78337e0] [00000004] 0x4 (unreliable)
> [e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
> [e78338f8] [c08a7158] schedule+0x34/0xc8
> [e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
> [e7833958] [c08a7be4] wait_for_common+0xa0/0x164
> [e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
> [e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
> [e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
> [e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
> [e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
> [e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
> [e7833f08] [c006c410] kthread+0xf8/0x124
> [e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c
> 
> addr2line on c03aba00 points to crypto/testmgr.c:1335
> 
>     1327)  if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
>     1328)      vec->digest_error) {
>     1329)          /* Just using digest() */
>     1330)          ahash_request_set_callback(req, req_flags, crypto_req_done,
>     1331)                                     &wait);
>     1332)          ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
>     1333)          err = do_ahash_op(crypto_ahash_digest, req, &wait, cfg->nosimd);
>     1334)          if (err) {
> -> 1335)                  if (err == vec->digest_error)
>     1336)                          return 0;
>     1337)                  pr_err("alg: ahash: %s digest() failed on test vector %s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
>     1338)                         driver, vec_name, vec->digest_error, err,
>     1339)                         cfg->name);
>     1340)                  return err;
>     1341)          }
>     1342)          if (vec->digest_error) {
>     1343)                  pr_err("alg: ahash: %s digest() unexpectedly succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
>     1344)                         driver, vec_name, vec->digest_error, cfg->name);
>     1345)                  return -EINVAL;
>     1346)          }
>     1347)          goto result_ready;
>     1348)  }
> 
> Seems that for some reason driver does not receive the interrupt from HW,
> thus completion callback does not run.
> 
> Tried with or without current patch series, no change in behaviour.
> 
> If you cannot reproduce and don't have any idea, I'll try the hard way
> (git bisect).

I cannot reproduce, both mpc885 and mpc8321e boot fine, and don't have 
any idea at first.

I know the SEC1 behaves that way when you submit zero-length data.

Christophe

> 
> Thanks,
> Horia
>