Hello,
I don't know whether anyone is actually maintaining the Rockchip crypto driver
in drivers/crypto/rockchip/, but it's failing the improved crypto tests
that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
See the boot logs for RK3288 from the KernelCI job here:
https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
the wrong ciphertext on some scatterlist layouts.
You can reproduce by pulling from
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
Note that I don't have this hardware myself, so if it turns out that no one is
interested in fixing this anytime soon I'll instead have to propose disabling
these algorithms until they can be fixed.
Thanks,
- Eric
(add LAKML and arm-soc maintainers)
On Sat, 26 Jan 2019 at 22:05, Eric Biggers <[email protected]> wrote:
>
> Hello,
>
> I don't know whether anyone is actually maintaining the Rockchip crypto driver
> in drivers/crypto/rockchip/, but it's failing the improved crypto tests
> that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
>
> See the boot logs for RK3288 from the KernelCI job here:
>
> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
>
> alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
> alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
> alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
> alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
> alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
>
> In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
> ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
> the wrong ciphertext on some scatterlist layouts.
>
> You can reproduce by pulling from
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
> branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
> setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
>
> Note that I don't have this hardware myself, so if it turns out that no one is
> interested in fixing this anytime soon I'll instead have to propose disabling
> these algorithms until they can be fixed.
>
> Thanks,
>
> - Eric
(also add Ezequiel who has spent a bit of time on the rockchip crypto
driver recently)
Am Sonntag, 27. Januar 2019, 09:54:54 CET schrieb Ard Biesheuvel:
> (add LAKML and arm-soc maintainers)
>
> On Sat, 26 Jan 2019 at 22:05, Eric Biggers <[email protected]> wrote:
> >
> > Hello,
> >
> > I don't know whether anyone is actually maintaining the Rockchip crypto driver
> > in drivers/crypto/rockchip/, but it's failing the improved crypto tests
> > that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
> >
> > See the boot logs for RK3288 from the KernelCI job here:
> >
> > https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
> > https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
> >
> > alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
> > alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
> > alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
> > alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
> > alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
> > alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
> >
> > In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
> > ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
> > the wrong ciphertext on some scatterlist layouts.
> >
> > You can reproduce by pulling from
> > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
> > branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
> > setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
> >
> > Note that I don't have this hardware myself, so if it turns out that no one is
> > interested in fixing this anytime soon I'll instead have to propose disabling
> > these algorithms until they can be fixed.
> >
> > Thanks,
> >
> > - Eric
>
Hi Eric and Heiko:
>> On Sat, 26 Jan 2019 at 22:05, Eric Biggers <[email protected]> wrote:
>>>
>>> Hello,
>>>
>>> I don't know whether anyone is actually maintaining the Rockchip crypto driver
>>> in drivers/crypto/rockchip/, but it's failing the improved crypto tests
>>> that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
Zhang Zhijie, engineer from Rockchip, will try to fix this software bug.
>>>
>>> See the boot logs for RK3288 from the KernelCI job here:
>>>
>>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
>>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
>>>
>>> alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
>>> alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
>>> alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
>>> alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
>>> alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
>>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
>>>
>>> In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
>>> ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
>>> the wrong ciphertext on some scatterlist layouts.
>>>
>>> You can reproduce by pulling from
>>> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
>>> branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
>>> setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
>>>
>>> Note that I don't have this hardware myself, so if it turns out that no one is
>>> interested in fixing this anytime soon I'll instead have to propose disabling
>>> these algorithms until they can be fixed.
>>>
>>> Thanks,
>>>
>>> - Eric
>>
>
Hi Zhang,
On Mon, Jan 28, 2019 at 11:14:32AM +0800, Tao Huang wrote:
> Hi Eric and Heiko:
>
> >> On Sat, 26 Jan 2019 at 22:05, Eric Biggers <[email protected]> wrote:
> >>>
> >>> Hello,
> >>>
> >>> I don't know whether anyone is actually maintaining the Rockchip crypto driver
> >>> in drivers/crypto/rockchip/, but it's failing the improved crypto tests
> >>> that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
>
> Zhang Zhijie, engineer from Rockchip, will try to fix this software bug.
>
> >>>
> >>> See the boot logs for RK3288 from the KernelCI job here:
> >>>
> >>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
> >>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
> >>>
> >>> alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
> >>> alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
> >>> alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
> >>> alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
> >>> alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
> >>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
> >>>
> >>> In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
> >>> ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
> >>> the wrong ciphertext on some scatterlist layouts.
> >>>
> >>> You can reproduce by pulling from
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
> >>> branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
> >>> setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
> >>>
> >>> Note that I don't have this hardware myself, so if it turns out that no one is
> >>> interested in fixing this anytime soon I'll instead have to propose disabling
> >>> these algorithms until they can be fixed.
> >>>
> >>> Thanks,
> >>>
> >>> - Eric
> >>
Thanks for the fixes, but I've improved the self-tests more, and there is
another bug. See the KernelCI job here:
https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.0-11071-g7d597cc3f0ef/
The self-tests are failing on the rk3288-rock2-square platform:
alg: skcipher: cbc-aes-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
alg: skcipher: cbc-des-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
The issue is that the self-tests now verify that CBC implementations update the
IV buffer to contain the next IV, aka the last ciphertext block. But the
Rockchip crypto driver doesn't do that, so it needs to be fixed.
This has always been a requirement for CBC implementations so that users can
chain CBC requests. Unfortunately it was just never tested for...
This should be easily reproducible using the mainline kernel.
- Eric
Adding my colleague Gael, who has been working on fixing this driver.
On Friday, March 15, 2019 00:31 -03, Eric Biggers <[email protected]> wrote:
> Hi Zhang,
>
> On Mon, Jan 28, 2019 at 11:14:32AM +0800, Tao Huang wrote:
> > Hi Eric and Heiko:
> >
> > >> On Sat, 26 Jan 2019 at 22:05, Eric Biggers <[email protected]> wrote:
> > >>>
> > >>> Hello,
> > >>>
> > >>> I don't know whether anyone is actually maintaining the Rockchip crypto driver
> > >>> in drivers/crypto/rockchip/, but it's failing the improved crypto tests
> > >>> that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
> >
> > Zhang Zhijie, engineer from Rockchip, will try to fix this software bug.
> >
> > >>>
> > >>> See the boot logs for RK3288 from the KernelCI job here:
> > >>>
> > >>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
> > >>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
> > >>>
> > >>> alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
> > >>> alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
> > >>> alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
> > >>> alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
> > >>> alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
> > >>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
> > >>>
> > >>> In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
> > >>> ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
> > >>> the wrong ciphertext on some scatterlist layouts.
> > >>>
> > >>> You can reproduce by pulling from
> > >>> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
> > >>> branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
> > >>> setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
> > >>>
> > >>> Note that I don't have this hardware myself, so if it turns out that no one is
> > >>> interested in fixing this anytime soon I'll instead have to propose disabling
> > >>> these algorithms until they can be fixed.
> > >>>
> > >>> Thanks,
> > >>>
> > >>> - Eric
> > >>
>
> Thanks for the fixes, but I've improved the self-tests more, and there is
> another bug. See the KernelCI job here:
>
> https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.0-11071-g7d597cc3f0ef/
>
> The self-tests are failing on the rk3288-rock2-square platform:
>
> alg: skcipher: cbc-aes-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
> alg: skcipher: cbc-des-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
>
> The issue is that the self-tests now verify that CBC implementations update the
> IV buffer to contain the next IV, aka the last ciphertext block. But the
> Rockchip crypto driver doesn't do that, so it needs to be fixed.
>
> This has always been a requirement for CBC implementations so that users can
> chain CBC requests. Unfortunately it was just never tested for...
>
> This should be easily reproducible using the mainline kernel.
>
> - Eric
Hello,
On 3/16/19 6:31 PM, Ezequiel Garcia wrote:
> Adding my colleague Gael, who has been working on fixing this driver.
>
I have a couple of pending commits that may fix that issue.
I will give it a try, and get back to you then.
> On Friday, March 15, 2019 00:31 -03, Eric Biggers <[email protected]> wrote:
>
>> Hi Zhang,
>>
>> On Mon, Jan 28, 2019 at 11:14:32AM +0800, Tao Huang wrote:
>>> Hi Eric and Heiko:
>>>
>>>>> On Sat, 26 Jan 2019 at 22:05, Eric Biggers <[email protected]> wrote:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> I don't know whether anyone is actually maintaining the Rockchip crypto driver
>>>>>> in drivers/crypto/rockchip/, but it's failing the improved crypto tests
>>>>>> that I currently have out for review: https://patchwork.kernel.org/cover/10778089/
>>>
>>> Zhang Zhijie, engineer from Rockchip, will try to fix this software bug.
>>>
>>>>>>
>>>>>> See the boot logs for RK3288 from the KernelCI job here:
>>>>>>
>>>>>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-rock2-square.txt
>>>>>> https://storage.kernelci.org/ardb/for-kernelci/v5.0-rc1-86-geaffe22db9d1/arm/multi_v7_defconfig/lab-collabora/boot-rk3288-veyron-jaq.txt
>>>>>>
>>>>>> alg: skcipher: ecb-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: use_digest src_divs=[15.64%@+3258, 84.36%@+4059] dst_divs=[69.11%@+1796, 8.49%@+4027, 6.34%@+1, 16.6%@+4058] iv_offset=21\"
>>>>>> alg: skcipher: cbc-aes-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@alignmask+3993] dst_divs=[65.31%@alignmask+1435, 34.69%@+14]\"
>>>>>> alg: skcipher: ecb-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_final src_divs=[<flush> 66.52%@+11, 33.48%@+1519] dst_divs=[58.82%@+1, 19.43%@+4082, 21.75%@+8]\"
>>>>>> alg: skcipher: cbc-des-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_finup src_divs=[100.0%@+3980] dst_divs=[60.4%@+3763, 23.9%@+4011, 16.87%@+4046]\"
>>>>>> alg: skcipher: ecb-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"random: may_sleep use_digest src_divs=[100.0%@+4] dst_divs=[47.25%@+19, 14.83%@+22, 37.92%@+31]\"
>>>>>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong result) on test vector 0, cfg=\"two even aligned splits\"
>>>>>>
>>>>>> In other words: the ecb-aes-rk, cbc-aes-rk, ecb-des-rk, cbc-des-rk,
>>>>>> ecb-des3-ede-rk, and cbc-des3-ede-rk algorithms are failing because they produce
>>>>>> the wrong ciphertext on some scatterlist layouts.
>>>>>>
>>>>>> You can reproduce by pulling from
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git
>>>>>> branch "testmgr-improvements", unsetting CONFIG_CRYPTO_MANAGER_DISABLE_TESTS,
>>>>>> setting CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y, rebooting and checking dmesg.
>>>>>>
>>>>>> Note that I don't have this hardware myself, so if it turns out that no one is
>>>>>> interested in fixing this anytime soon I'll instead have to propose disabling
>>>>>> these algorithms until they can be fixed.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> - Eric
>>>>>
>>
>> Thanks for the fixes, but I've improved the self-tests more, and there is
>> another bug. See the KernelCI job here:
>>
>> https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.0-11071-g7d597cc3f0ef/
>>
>> The self-tests are failing on the rk3288-rock2-square platform:
>>
>> alg: skcipher: cbc-aes-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
>> alg: skcipher: cbc-des-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong output IV) on test vector 0, cfg=\"in-place\"
>>
>> The issue is that the self-tests now verify that CBC implementations update the
>> IV buffer to contain the next IV, aka the last ciphertext block. But the
>> Rockchip crypto driver doesn't do that, so it needs to be fixed.
>>
>> This has always been a requirement for CBC implementations so that users can
>> chain CBC requests. Unfortunately it was just never tested for...
>>
>> This should be easily reproducible using the mainline kernel.
>>
>> - Eric
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Gael
Hi All,
I'm looking further to debug an issue regarding CRC32 selftests. I'm
currently blocked on an issue about the second test vector implemented
on CRC32:
static const struct hash_testvec crc32_tv_template[] = {
{
.plaintext = "abcdefg",
.psize = 7,
.digest = "\xd8\xb5\x46\xac",
},
I'm currently trying to understand the issue, but using others tools
(computer or Online CRC calculation), I'm not able to find any way to
get the expected output? This new vector was introduced for few version
but I'm wondering who was able to verify it.
Should it be written by byte? Word? Half word?
Thanks for help.
BR,
Lionel
On Thu, Mar 21, 2019 at 10:46:53AM +0000, Lionel DEBIEVE wrote:
> Hi All,
>
> I'm looking further to debug an issue regarding CRC32 selftests. I'm
> currently blocked on an issue about the second test vector implemented
> on CRC32:
> static const struct hash_testvec crc32_tv_template[] = {
> ??? {
> ??? ??? .plaintext = "abcdefg",
> ??? ??? .psize = 7,
>
> ??? ??? .digest = "\xd8\xb5\x46\xac",
>
> ??? },
>
> I'm currently trying to understand the issue, but using others tools
> (computer or Online CRC calculation), I'm not able to find any way to
> get the expected output? This new vector was introduced for few version
> but I'm wondering who was able to verify it.
>
> Should it be written by byte? Word? Half word?
>
> Thanks for help.
>
> BR,
> Lionel
It's the CRC-32 of the 7-byte buffer "abcdefg" using an initial value of 0.
Bitwise little endian with a little endian digest, and no complementation at the
beginning or end. Note that the initial value ("key") is not given explicitly,
i.e. this tests that the implementation uses the correct default initial value.
The generic, arm, and x86 implementations in the kernel obviously all pass this,
otherwise people would be complaining about them. You can also verify it in
userspace with zlib using:
#include <assert.h>
#include <stdint.h>
#include <zlib.h>
int main()
{
assert((uint32_t)~crc32(~0, "abcdefg", 7) == 0xac46b5d8);
}
(zlib complements the value at the beginning and end, so it must be undone to
match the kernel conversion which doesn't do that.)
- Eric
Hello,
On 3/18/19 11:03 AM, Gael PORTAY wrote:
> Hello,
>
> On 3/16/19 6:31 PM, Ezequiel Garcia wrote:
>> Adding my colleague Gael, who has been working on fixing this driver.
>
> I have a couple of pending commits that may fix that issue.
>
> I will give it a try, and get back to you then.
>
The patches I had fix the same issue than recent commit to [1] and [2]
in a different way.
But they do not fix the issue below.
>> ...
>>>
>>> Thanks for the fixes, but I've improved the self-tests more, and
>>> there is
>>> another bug. See the KernelCI job here:
>>>
>>> https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.0-11071-g7d597cc3f0ef/
>>>
>>>
>>> The self-tests are failing on the rk3288-rock2-square platform:
>>>
>>> alg: skcipher: cbc-aes-rk encryption test failed (wrong output
>>> IV) on test vector 0, cfg=\"in-place\"
>>> alg: skcipher: cbc-des-rk encryption test failed (wrong output
>>> IV) on test vector 0, cfg=\"in-place\"
>>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong
>>> output IV) on test vector 0, cfg=\"in-place\"
>>>
>>> The issue is that the self-tests now verify that CBC implementations
>>> update the
>>> IV buffer to contain the next IV, aka the last ciphertext block. But
>>> the
>>> Rockchip crypto driver doesn't do that, so it needs to be fixed.
>>>
>>> This has always been a requirement for CBC implementations so that
>>> users can
>>> chain CBC requests. Unfortunately it was just never tested for...
>>>
>>> This should be easily reproducible using the mainline kernel.
>>>
>>> - Eric
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> Gael
[1]:
https://github.com/torvalds/linux/commit/c1c214adcb56d36433480c8fedf772498e7e539c#diff-440313f9d25f65c14d4bffb1360a3c60
[2]:
https://github.com/torvalds/linux/commit/4359669a087633132203c52d67dd8c31e09e7b2e#diff-440313f9d25f65c14d4bffb1360a3c60
Gael
Hi Eric and Gael,
On 2019/3/22 上午1:04, Gael PORTAY wrote:
> Hello,
>
> On 3/18/19 11:03 AM, Gael PORTAY wrote:
>> Hello,
>>
>> On 3/16/19 6:31 PM, Ezequiel Garcia wrote:
>>> Adding my colleague Gael, who has been working on fixing this driver.
>>
>> I have a couple of pending commits that may fix that issue.
>>
>> I will give it a try, and get back to you then.
>>
>
> The patches I had fix the same issue than recent commit to [1] and [2]
> in a different way.
>
> But they do not fix the issue below.
I will try to fix the issue below.
>
>>> ...
>>>>
>>>> Thanks for the fixes, but I've improved the self-tests more, and
>>>> there is
>>>> another bug. See the KernelCI job here:
>>>>
>>>> https://kernelci.org/boot/all/job/ardb/branch/for-kernelci/kernel/v5.0-11071-g7d597cc3f0ef/
>>>>
>>>>
>>>> The self-tests are failing on the rk3288-rock2-square platform:
>>>>
>>>> alg: skcipher: cbc-aes-rk encryption test failed (wrong output
>>>> IV) on test vector 0, cfg=\"in-place\"
>>>> alg: skcipher: cbc-des-rk encryption test failed (wrong output
>>>> IV) on test vector 0, cfg=\"in-place\"
>>>> alg: skcipher: cbc-des3-ede-rk encryption test failed (wrong
>>>> output IV) on test vector 0, cfg=\"in-place\"
>>>>
>>>> The issue is that the self-tests now verify that CBC
>>>> implementations update the
>>>> IV buffer to contain the next IV, aka the last ciphertext block.
>>>> But the
>>>> Rockchip crypto driver doesn't do that, so it needs to be fixed.
>>>>
>>>> This has always been a requirement for CBC implementations so that
>>>> users can
>>>> chain CBC requests. Unfortunately it was just never tested for...
>>>>
>>>> This should be easily reproducible using the mainline kernel.
>>>>
>>>> - Eric
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> [email protected]
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>> Gael
>
> [1]:
> https://github.com/torvalds/linux/commit/c1c214adcb56d36433480c8fedf772498e7e539c#diff-440313f9d25f65c14d4bffb1360a3c60
> [2]:
> https://github.com/torvalds/linux/commit/4359669a087633132203c52d67dd8c31e09e7b2e#diff-440313f9d25f65c14d4bffb1360a3c60
>
> Gael
>
>
> The issue is that the self-tests now verify that CBC implementations update the
> IV buffer to contain the next IV, aka the last ciphertext block. But the Rockchip
> crypto driver doesn't do that, so it needs to be fixed.
>
> This has always been a requirement for CBC implementations so that users can
> chain CBC requests. Unfortunately it was just never tested for...
>
This did not immediately trigger me when it came flying past a couple of weeks
ago, but I ran into the same issue today with the inside_secure driver I'm playing
with: it does NOT return correct IV outputs for CBC modes.
However ... I'd like to question that very requirement ... if I may :-)
My reasoning is that this IV output *is* available as the last block of either the
output (for encrypt) or input (for decrypt) datastream. So requiring it to be
updated in the IV buffer as well seems redundant to me. It burdens the driver
with an extra data copy operation, while in the majority of practicle use cases
you would not even *need* this output IV. (chaining IV's would not work on
a hardware accelerator anyway, because you would need to serialize the
datastream, meaning you run at the speed of the round-trip latency instead
of the throughput, which is typically one to two orders of a magnitude slower)
And in the odd case you do need it, you can just grab it from the data buffer
yourself.
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines
On Thu, Apr 04, 2019 at 01:41:50PM +0000, Pascal Van Leeuwen wrote:
> > The issue is that the self-tests now verify that CBC implementations update the
> > IV buffer to contain the next IV, aka the last ciphertext block. But the Rockchip
> > crypto driver doesn't do that, so it needs to be fixed.
> >
> > This has always been a requirement for CBC implementations so that users can
> > chain CBC requests. Unfortunately it was just never tested for...
> >
> This did not immediately trigger me when it came flying past a couple of weeks
> ago, but I ran into the same issue today with the inside_secure driver I'm playing
> with: it does NOT return correct IV outputs for CBC modes.
>
> However ... I'd like to question that very requirement ... if I may :-)
>
> My reasoning is that this IV output *is* available as the last block of either the
> output (for encrypt) or input (for decrypt) datastream. So requiring it to be
> updated in the IV buffer as well seems redundant to me. It burdens the driver
> with an extra data copy operation, while in the majority of practicle use cases
> you would not even *need* this output IV. (chaining IV's would not work on
> a hardware accelerator anyway, because you would need to serialize the
> datastream, meaning you run at the speed of the round-trip latency instead
> of the throughput, which is typically one to two orders of a magnitude slower)
>
> And in the odd case you do need it, you can just grab it from the data buffer
> yourself.
>
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines
>
Herbert, can you explain what users actually rely on the next IV being returned?
I don't know all the historical context behind this.
- Eric
On Thu, Apr 04, 2019 at 10:12:05AM -0700, Eric Biggers wrote:
>
> Herbert, can you explain what users actually rely on the next IV being returned?
> I don't know all the historical context behind this.
Chaining by reusing the output IV has always been part of the API.
However, we never tested it in testmgr though so a number of drivers
are broken in this regard.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> > Herbert, can you explain what users actually rely on the next IV being
> returned?
> > I don't know all the historical context behind this.
>
> Chaining by reusing the output IV has always been part of the API.
>
Then where is this specified? Because I read through all the Kernel Crypto
API documentation multiple times, but I have not been able to find ANY
reference to any output IV being stored anywhere.
Yes, I can infer from the source code that this is what the standard CBC
wrapper does, but that may just as well have been a side-effect of that
particular implementation.
Fact is, there are at least 2 hardware device drivers NOT doing this - and
I want to bet a nice sum of money there will be more - and that this has
not been noticed prior to adding these tests to testmgr, otherwise this
would have been fixed by now. Which seems to confirm that there is no
real use case for this functionality.
So why would we fail and disable crypto drivers that have been working
perfectly fine so far on their true use cases just because of some purely
theoretical "part of the API" that may just as well be personal opinion?
Or add some expensive work-around to the driver, for that matter.
Expensive, because the driver will have to compute the last crypto block
offset, walk the scatter chain, copy the last block data while taking into
account the corner case where this data is crossing scatter blocks.
That's a lot of overhead for something that has no real use case.
Regards,
Pascal
On Sun, Apr 07, 2019 at 07:12:43PM +0000, Pascal Van Leeuwen wrote:
>
> Then where is this specified? Because I read through all the Kernel Crypto
> API documentation multiple times, but I have not been able to find ANY
> reference to any output IV being stored anywhere.
> Yes, I can infer from the source code that this is what the standard CBC
> wrapper does, but that may just as well have been a side-effect of that
> particular implementation.
Patches to improve the documentation are welcome.
> So why would we fail and disable crypto drivers that have been working
> perfectly fine so far on their true use cases just because of some purely
> theoretical "part of the API" that may just as well be personal opinion?
Any drivers not obeying this API rule will be broken when used
in conjunction with algif_skcipher as well as templates such as
cts.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> > Then where is this specified? Because I read through all the Kernel
> > Crypto API documentation multiple times, but I have not been able to
> > find ANY reference to any output IV being stored anywhere.
> > Yes, I can infer from the source code that this is what the standard
> > CBC wrapper does, but that may just as well have been a side-effect of
> > that particular implementation.
>
> Patches to improve the documentation are welcome.
>
How should someone who did not architect the API himself know the
documentation is wrong in this regard? The documentation is the only thing
I have available to go by ...
> Any drivers not obeying this API rule will be broken when used in conjunction
> with algif_skcipher as well as templates such as cts.
>
Ok, so removing it breaks some existing functionality, that might be a valid
reason to keep it in ... BUT these may still not be relevant use cases for a
certain hardware accelerator. Which was NOT a problem until the testmgr
started actually *failing* and *disabling* implementations on these features.
(the iv output is just the tip of the iceberg here, really ...)
So now implementors of hw crypto acceleration have 2 options:
1) tell their customers to keep the run-time self-tests disabled, which makes
them look bad
-or-
2) implement some convoluted work-around in the driver that will slow
down their actual main use cases to the point where the HW becomes useless
It would be nice to have some option in testmgr to just test the core algorithm
itself and not all the nitty gritty corners of the API that may not be relevant
i.e. split off the core algorithm cases (e.g. proper straightforward encryption
of a single block of data with a certain key) from the API cases.
Perhaps a driver could advertise this through some flag: "I'm not fully API
compliant, so just test the algorithm and not any API corner cases". Or even
"please don't test me, I will test myself".
An alternative approach would be capability flags to advertise specific API
features, but I can see how you can quickly go overboard with that.
But please do keep in mind that:
a) These crypto accelerators were NOT designed specifically for the Kernel
Crypto API, in fact, the majority of them predate it by quite some margin.
They may have very specific target use cases, like IPsec acceleration or
disk encryption and not be able to (efficiently) support other API features.
b) Some features are simply NOT suitable for HW acceleration. A HW centric
API would not advertise those, but we are where we are now ...
c) Working around a) or b) in the driver is sometimes possible, but you don't
want to do that for exception cases as it slows down the common case.
Regards,
Pascal
On Mon, Apr 08, 2019 at 08:59:02AM +0000, Pascal Van Leeuwen wrote:
>
> It would be nice to have some option in testmgr to just test the core algorithm
> itself and not all the nitty gritty corners of the API that may not be relevant
> i.e. split off the core algorithm cases (e.g. proper straightforward encryption
> of a single block of data with a certain key) from the API cases.
> Perhaps a driver could advertise this through some flag: "I'm not fully API
> compliant, so just test the algorithm and not any API corner cases". Or even
> "please don't test me, I will test myself".
>
> An alternative approach would be capability flags to advertise specific API
> features, but I can see how you can quickly go overboard with that.
What we could do is have the user specify an explicit flag saying
that they do not care about the output IV. You could then skip the
output IV step in your driver.
But you'll have to code this up, including the bit on the user-side
to actually set the flag.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Apr 08, 2019 at 08:59:02AM +0000, Pascal Van Leeuwen wrote:
> > > Then where is this specified? Because I read through all the Kernel
> > > Crypto API documentation multiple times, but I have not been able to
> > > find ANY reference to any output IV being stored anywhere.
> > > Yes, I can infer from the source code that this is what the standard
> > > CBC wrapper does, but that may just as well have been a side-effect of
> > > that particular implementation.
> >
> > Patches to improve the documentation are welcome.
> >
> How should someone who did not architect the API himself know the
> documentation is wrong in this regard? The documentation is the only thing
> I have available to go by ...
>
> > Any drivers not obeying this API rule will be broken when used in conjunction
> > with algif_skcipher as well as templates such as cts.
> >
> Ok, so removing it breaks some existing functionality, that might be a valid
> reason to keep it in ... BUT these may still not be relevant use cases for a
> certain hardware accelerator. Which was NOT a problem until the testmgr
> started actually *failing* and *disabling* implementations on these features.
> (the iv output is just the tip of the iceberg here, really ...)
>
> So now implementors of hw crypto acceleration have 2 options:
> 1) tell their customers to keep the run-time self-tests disabled, which makes
> them look bad
> -or-
> 2) implement some convoluted work-around in the driver that will slow
> down their actual main use cases to the point where the HW becomes useless
>
> It would be nice to have some option in testmgr to just test the core algorithm
> itself and not all the nitty gritty corners of the API that may not be relevant
> i.e. split off the core algorithm cases (e.g. proper straightforward encryption
> of a single block of data with a certain key) from the API cases.
> Perhaps a driver could advertise this through some flag: "I'm not fully API
> compliant, so just test the algorithm and not any API corner cases". Or even
> "please don't test me, I will test myself".
>
> An alternative approach would be capability flags to advertise specific API
> features, but I can see how you can quickly go overboard with that.
>
> But please do keep in mind that:
> a) These crypto accelerators were NOT designed specifically for the Kernel
> Crypto API, in fact, the majority of them predate it by quite some margin.
> They may have very specific target use cases, like IPsec acceleration or
> disk encryption and not be able to (efficiently) support other API features.
> b) Some features are simply NOT suitable for HW acceleration. A HW centric
> API would not advertise those, but we are where we are now ...
> c) Working around a) or b) in the driver is sometimes possible, but you don't
> want to do that for exception cases as it slows down the common case.
>
One person's "corner case" is another person's "use case". Once you register
your driver with the crypto API, you can't control how it's used. If you
provide "cbc(aes)", for example, anyone in the kernel may get your driver when
they ask the crypto API to do "cbc(aes)" encryption or decryption.
Correctness comes first, and there's no such thing as "testing the algorithm,
not the API", since the API is means by which the algorithm is accessed. So you
*must* implement the API correctly. If you don't, it's very much Working As
Intended for the self-tests to disable your driver.
If you aren't happy with the API, then please work with the community to improve
the API instead. E.g. perhaps we should annotate with a new request flag all
users who may actually need the IV chaining behavior. Then on all other
requests, implementations wouldn't be required to provide the next IV. It would
add complexity, but perhaps it would be worthwhile.
Remember that in the case of unsupported lengths, keys, or data layouts, you
also have the option of using a fallback algorithm to handle those cases. Grep
for CRYPTO_ALG_NEED_FALLBACK to see examples in other drivers.
- Eric
On Sun, Apr 07, 2019 at 07:12:43PM +0000, Pascal Van Leeuwen wrote:
>
> Fact is, there are at least 2 hardware device drivers NOT doing this - and
> I want to bet a nice sum of money there will be more - and that this has
> not been noticed prior to adding these tests to testmgr, otherwise this
> would have been fixed by now. Which seems to confirm that there is no
> real use case for this functionality.
>
I really shouldn't have to say this, but just because something hasn't been
reported doesn't mean it's not a real problem. Someone could easily be affected
by one of these bugs where crypto drivers produce the wrong output, and never
notice it because their use case doesn't involve checking the output against
another implementation. Or, perhaps they noticed but never reported it
upstream. Or perhaps they didn't have the time or skill to debug the problem so
just they disabled the broken driver, or used No Crypto instead.
That's why we have tests -- so bugs can be detected immediately rather than
maybe years out in the field after causing critical security vulnerabilities.
- Eric
On Mon, 8 Apr 2019 at 20:28, Eric Biggers <[email protected]> wrote:
>
> On Sun, Apr 07, 2019 at 07:12:43PM +0000, Pascal Van Leeuwen wrote:
> >
> > Fact is, there are at least 2 hardware device drivers NOT doing this - and
> > I want to bet a nice sum of money there will be more - and that this has
> > not been noticed prior to adding these tests to testmgr, otherwise this
> > would have been fixed by now. Which seems to confirm that there is no
> > real use case for this functionality.
> >
>
> I really shouldn't have to say this, but just because something hasn't been
> reported doesn't mean it's not a real problem. Someone could easily be affected
> by one of these bugs where crypto drivers produce the wrong output, and never
> notice it because their use case doesn't involve checking the output against
> another implementation. Or, perhaps they noticed but never reported it
> upstream. Or perhaps they didn't have the time or skill to debug the problem so
> just they disabled the broken driver, or used No Crypto instead.
>
> That's why we have tests -- so bugs can be detected immediately rather than
> maybe years out in the field after causing critical security vulnerabilities.
>
Perhaps we could wire this up using a CRYPTO_ALG flag? I.e.,
implementations that need the compliant behavior (such as cts) check
for the flag, and the asynchronous implementations produce two
versions, where the one that lacks the flag has a higher priority.
That way, we don't take the performance hit in cases where it is not
needed.
>
> What we could do is have the user specify an explicit flag saying
> that they do not care about the output IV. You could then skip the
> output IV step in your driver.
>
That would work for me, if the maintainers would be OK with adding
such flags.
Also, as a heads up - just to get other peoples opinion here - I
might prefer responding to such a flag by selecting the fallback
cipher instead of actually implementing a workaround for my hardware
in case such a workaround would be detrimental to the performance.
> But you'll have to code this up, including the bit on the user-side
> to actually set the flag.
>
Since that user side probably lives in the kernel tree too, I could
do that. This would then apply to testmgr as well though, as I need
to ensure it disables features for tests that don't require them.
(otherwise you'd be verifying only the fallback cipher)
Regards,
Pascal
> One person's "corner case" is another person's "use case". Once you
>
That argument works both ways though. Someone else's use case may be
getting decent performance from an accelerator for a specific
application, which has worked fine for years but is now broken to
comply with some (for them!) possibly irrelevant corner of the API.
> register
> your driver with the crypto API, you can't control how it's used. If
> you
> provide "cbc(aes)", for example, anyone in the kernel may get your
> driver when
> they ask the crypto API to do "cbc(aes)" encryption or decryption.
>
I understand that's how it currently works. So maybe we need some
more control from the driver there, as you suggested below yourself.
> Correctness comes first, and there's no such thing as "testing the
> algorithm,
> not the API", since the API is means by which the algorithm is
> accessed. So you
> *must* implement the API correctly. If you don't, it's very much
> Working As
> Intended for the self-tests to disable your driver.
>
But, lacking a formal specification, who decides what is a "correct"
implementation of the API?
> If you aren't happy with the API, then please work with the community
> to improve
> the API instead.
>
That's exactly the plan :-) And I'm not unhappy, by the way ;-)
> E.g. perhaps we should annotate with a new request
> flag all
> users who may actually need the IV chaining behavior. Then on all
> other
> requests, implementations wouldn't be required to provide the next IV.
> It would
> add complexity, but perhaps it would be worthwhile.
>
There was another suggestion along those lines, but then inverted,
like the user requesting that it does NOT need the IV chaining.
I like that better as it would be fully backward compatible and
you could change just the users that would actually benefit and
leave everything else untouched.
> Remember that in the case of unsupported lengths, keys, or data
> layouts, you
> also have the option of using a fallback algorithm to handle those
> cases. Grep
> for CRYPTO_ALG_NEED_FALLBACK to see examples in other drivers.
>
Yes, except that I cannot fallback because of the output IV thing
if I don't know the user is going to need the output IV.
I surely don't want to fallback by default! So I need that flag.
Regards,
Pascal
> I really shouldn't have to say this, but just because something hasn't
> been
> reported doesn't mean it's not a real problem. Someone could easily be
> affected
> by one of these bugs where crypto drivers produce the wrong output, and
> never
> notice it because their use case doesn't involve checking the output
> against
> another implementation. Or, perhaps they noticed but never reported it
> upstream. Or perhaps they didn't have the time or skill to debug the
> problem so
> just they disabled the broken driver, or used No Crypto instead.
>
> That's why we have tests -- so bugs can be detected immediately rather
> than
> maybe years out in the field after causing critical security
> vulnerabilities.
>
I understand where you're coming from. My first assumption was that
perhaps this corner of the API was not used at all, in which case the
specification and testmgr could simply be updated to remove it.
I suppose that's not entirely the case. But the few (?) users relying
on this functionality could still be changed to use an alternative
approach such as extracting the output IV from the packet data so any
uses NOT needing this output IV are not bothered with it. That may
actually help the performance of software implementations as well.
Somehow, somewhere, that data must be duplicated, which takes time.
In any, we already came up with the alternative approach of having
some kind of "I_DONT_NEED_IVOUT" flag provided by the user.
Regards,
Pascal