mbox series

[00/10] crypto: omap fixes towards 5.5

Message ID 20191017122549.4634-1-t-kristo@ti.com (mailing list archive)
Headers show
Series crypto: omap fixes towards 5.5 | expand

Message

Tero Kristo Oct. 17, 2019, 12:25 p.m. UTC
Hi,

This series fixes a number of bugs with omap crypto implementation.
These have become evident with the changes to the cryptomanager, where
it adds some new test cases and modifies some existing, namely the split
update tests. Also, while fixing the cryptomanager induced bugs, some
other surfaced with tcrypt/IPSec tests, so fixed them aswell.

Patch #9 is against crypto core modifying the crypto_wait_req
common API to have a timeout for it also, currently it waits forever
and it is kind of difficult to see what test fails with crypto manager.
This is not really needed for anything, but it is kind of nice to have
(makes debugging easier.)

This series has been tested on top of 5.4-rc2, with following setups,
on AM57xx-beagle-x15 board:

- crypto manager self tests
- tcrypt performance test
- ipsec test with strongswan

This series depends on the skcipher API switch patch from Ard Biesheuvel
[1].

-Tero

[1] https://patchwork.kernel.org/patch/11188595/


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Ard Biesheuvel Oct. 25, 2019, 11:33 a.m. UTC | #1
On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>
> Hi,
>
> This series fixes a number of bugs with omap crypto implementation.
> These have become evident with the changes to the cryptomanager, where
> it adds some new test cases and modifies some existing, namely the split
> update tests. Also, while fixing the cryptomanager induced bugs, some
> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>
> Patch #9 is against crypto core modifying the crypto_wait_req
> common API to have a timeout for it also, currently it waits forever
> and it is kind of difficult to see what test fails with crypto manager.
> This is not really needed for anything, but it is kind of nice to have
> (makes debugging easier.)
>
> This series has been tested on top of 5.4-rc2, with following setups,
> on AM57xx-beagle-x15 board:
>
> - crypto manager self tests
> - tcrypt performance test
> - ipsec test with strongswan
>
> This series depends on the skcipher API switch patch from Ard Biesheuvel
> [1].
>

Hi Tero,

On my BeagleBone White, I am hitting the following issues after
applying these patches:

[    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
succeeded on test vector "random: len=531 klen=32";
expected_error=-22, cfg="random: inplace may_sleep use_finup
src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
21.13%@+2728]"
[    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
succeeded on test vector "random: len=1118 klen=32";
expected_error=-22, cfg="random: may_sleep use_final
src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"

These are simply a result of the ECB and CBC implementations not
returning -EINVAL when the input is not a multiple of the block size.

[    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
match generic impl (1)

This means cra_blocksize is not set to 1 as it should. If your driver
uses the skcipher walk API, it should set the walksize to
AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
don't, then you can disregard that part.

[    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
expected_error=-22

Another missing sanity check. GCM only permits certain authsizes.

[    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
unaligned cases.

This is not a bug, but I'm not sure if the below is related or not.

I'll preserve the binaries, in case you need me to objdump anything.
Tero Kristo Oct. 25, 2019, 11:55 a.m. UTC | #2
On 25/10/2019 14:33, Ard Biesheuvel wrote:
> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>>
>> Hi,
>>
>> This series fixes a number of bugs with omap crypto implementation.
>> These have become evident with the changes to the cryptomanager, where
>> it adds some new test cases and modifies some existing, namely the split
>> update tests. Also, while fixing the cryptomanager induced bugs, some
>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>>
>> Patch #9 is against crypto core modifying the crypto_wait_req
>> common API to have a timeout for it also, currently it waits forever
>> and it is kind of difficult to see what test fails with crypto manager.
>> This is not really needed for anything, but it is kind of nice to have
>> (makes debugging easier.)
>>
>> This series has been tested on top of 5.4-rc2, with following setups,
>> on AM57xx-beagle-x15 board:
>>
>> - crypto manager self tests
>> - tcrypt performance test
>> - ipsec test with strongswan
>>
>> This series depends on the skcipher API switch patch from Ard Biesheuvel
>> [1].
>>
> 
> Hi Tero,
> 
> On my BeagleBone White, I am hitting the following issues after
> applying these patches:
> 
> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
> succeeded on test vector "random: len=531 klen=32";
> expected_error=-22, cfg="random: inplace may_sleep use_finup
> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
> 21.13%@+2728]"
> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
> succeeded on test vector "random: len=1118 klen=32";
> expected_error=-22, cfg="random: may_sleep use_final
> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
> 
> These are simply a result of the ECB and CBC implementations not
> returning -EINVAL when the input is not a multiple of the block size.
> 
> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
> match generic impl (1)
> 
> This means cra_blocksize is not set to 1 as it should. If your driver
> uses the skcipher walk API, it should set the walksize to
> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
> don't, then you can disregard that part.
> 
> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
> expected_error=-22
> 
> Another missing sanity check. GCM only permits certain authsizes.
> 
> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
> unaligned cases.
> 
> This is not a bug, but I'm not sure if the below is related or not.
> 
> I'll preserve the binaries, in case you need me to objdump anything.

What are these tests you are executing? For me, the testmgr self test 
suite is passing just fine. Any extra tests you have enabled somehow?

I am also running full test on different board though (am57xx), I 
haven't been explicitly running anything on am335x.

-Tero


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Tero Kristo Oct. 25, 2019, 11:56 a.m. UTC | #3
On 25/10/2019 14:55, Tero Kristo wrote:
> On 25/10/2019 14:33, Ard Biesheuvel wrote:
>> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>>>
>>> Hi,
>>>
>>> This series fixes a number of bugs with omap crypto implementation.
>>> These have become evident with the changes to the cryptomanager, where
>>> it adds some new test cases and modifies some existing, namely the split
>>> update tests. Also, while fixing the cryptomanager induced bugs, some
>>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>>>
>>> Patch #9 is against crypto core modifying the crypto_wait_req
>>> common API to have a timeout for it also, currently it waits forever
>>> and it is kind of difficult to see what test fails with crypto manager.
>>> This is not really needed for anything, but it is kind of nice to have
>>> (makes debugging easier.)
>>>
>>> This series has been tested on top of 5.4-rc2, with following setups,
>>> on AM57xx-beagle-x15 board:
>>>
>>> - crypto manager self tests
>>> - tcrypt performance test
>>> - ipsec test with strongswan
>>>
>>> This series depends on the skcipher API switch patch from Ard Biesheuvel
>>> [1].
>>>
>>
>> Hi Tero,
>>
>> On my BeagleBone White, I am hitting the following issues after
>> applying these patches:
>>
>> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
>> succeeded on test vector "random: len=531 klen=32";
>> expected_error=-22, cfg="random: inplace may_sleep use_finup
>> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
>> 21.13%@+2728]"
>> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
>> succeeded on test vector "random: len=1118 klen=32";
>> expected_error=-22, cfg="random: may_sleep use_final
>> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
>>
>> These are simply a result of the ECB and CBC implementations not
>> returning -EINVAL when the input is not a multiple of the block size.
>>
>> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
>> match generic impl (1)
>>
>> This means cra_blocksize is not set to 1 as it should. If your driver
>> uses the skcipher walk API, it should set the walksize to
>> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
>> don't, then you can disregard that part.
>>
>> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
>> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
>> expected_error=-22
>>
>> Another missing sanity check. GCM only permits certain authsizes.
>>
>> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
>> unaligned cases.
>>
>> This is not a bug, but I'm not sure if the below is related or not.
>>
>> I'll preserve the binaries, in case you need me to objdump anything.
> 
> What are these tests you are executing? For me, the testmgr self test 
> suite is passing just fine. Any extra tests you have enabled somehow?
> 
> I am also running full test on different board though (am57xx), I 
> haven't been explicitly running anything on am335x.

Oh, and btw, did you try without my series? I think the selftests are 
failing rather miserably without them...

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Ard Biesheuvel Oct. 25, 2019, 12:05 p.m. UTC | #4
On Fri, 25 Oct 2019 at 13:56, Tero Kristo <t-kristo@ti.com> wrote:
>
> On 25/10/2019 14:55, Tero Kristo wrote:
> > On 25/10/2019 14:33, Ard Biesheuvel wrote:
> >> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> This series fixes a number of bugs with omap crypto implementation.
> >>> These have become evident with the changes to the cryptomanager, where
> >>> it adds some new test cases and modifies some existing, namely the split
> >>> update tests. Also, while fixing the cryptomanager induced bugs, some
> >>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
> >>>
> >>> Patch #9 is against crypto core modifying the crypto_wait_req
> >>> common API to have a timeout for it also, currently it waits forever
> >>> and it is kind of difficult to see what test fails with crypto manager.
> >>> This is not really needed for anything, but it is kind of nice to have
> >>> (makes debugging easier.)
> >>>
> >>> This series has been tested on top of 5.4-rc2, with following setups,
> >>> on AM57xx-beagle-x15 board:
> >>>
> >>> - crypto manager self tests
> >>> - tcrypt performance test
> >>> - ipsec test with strongswan
> >>>
> >>> This series depends on the skcipher API switch patch from Ard Biesheuvel
> >>> [1].
> >>>
> >>
> >> Hi Tero,
> >>
> >> On my BeagleBone White, I am hitting the following issues after
> >> applying these patches:
> >>
> >> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
> >> succeeded on test vector "random: len=531 klen=32";
> >> expected_error=-22, cfg="random: inplace may_sleep use_finup
> >> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
> >> 21.13%@+2728]"
> >> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
> >> succeeded on test vector "random: len=1118 klen=32";
> >> expected_error=-22, cfg="random: may_sleep use_final
> >> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
> >>
> >> These are simply a result of the ECB and CBC implementations not
> >> returning -EINVAL when the input is not a multiple of the block size.
> >>
> >> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
> >> match generic impl (1)
> >>
> >> This means cra_blocksize is not set to 1 as it should. If your driver
> >> uses the skcipher walk API, it should set the walksize to
> >> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
> >> don't, then you can disregard that part.
> >>
> >> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
> >> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
> >> expected_error=-22
> >>
> >> Another missing sanity check. GCM only permits certain authsizes.
> >>
> >> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
> >> unaligned cases.
> >>
> >> This is not a bug, but I'm not sure if the below is related or not.
> >>
> >> I'll preserve the binaries, in case you need me to objdump anything.
> >
> > What are these tests you are executing? For me, the testmgr self test
> > suite is passing just fine. Any extra tests you have enabled somehow?
> >

I enabled CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, which enables a bunch of
fuzz tests of the offloaded algorithms against the generic
implementations.

> > I am also running full test on different board though (am57xx), I
> > haven't been explicitly running anything on am335x.
>
> Oh, and btw, did you try without my series? I think the selftests are
> failing rather miserably without them...
>

No, I just tried a branch with mine and your patches applied.
Tero Kristo Oct. 25, 2019, 12:18 p.m. UTC | #5
On 25/10/2019 15:05, Ard Biesheuvel wrote:
> On Fri, 25 Oct 2019 at 13:56, Tero Kristo <t-kristo@ti.com> wrote:
>>
>> On 25/10/2019 14:55, Tero Kristo wrote:
>>> On 25/10/2019 14:33, Ard Biesheuvel wrote:
>>>> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> This series fixes a number of bugs with omap crypto implementation.
>>>>> These have become evident with the changes to the cryptomanager, where
>>>>> it adds some new test cases and modifies some existing, namely the split
>>>>> update tests. Also, while fixing the cryptomanager induced bugs, some
>>>>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
>>>>>
>>>>> Patch #9 is against crypto core modifying the crypto_wait_req
>>>>> common API to have a timeout for it also, currently it waits forever
>>>>> and it is kind of difficult to see what test fails with crypto manager.
>>>>> This is not really needed for anything, but it is kind of nice to have
>>>>> (makes debugging easier.)
>>>>>
>>>>> This series has been tested on top of 5.4-rc2, with following setups,
>>>>> on AM57xx-beagle-x15 board:
>>>>>
>>>>> - crypto manager self tests
>>>>> - tcrypt performance test
>>>>> - ipsec test with strongswan
>>>>>
>>>>> This series depends on the skcipher API switch patch from Ard Biesheuvel
>>>>> [1].
>>>>>
>>>>
>>>> Hi Tero,
>>>>
>>>> On my BeagleBone White, I am hitting the following issues after
>>>> applying these patches:
>>>>
>>>> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
>>>> succeeded on test vector "random: len=531 klen=32";
>>>> expected_error=-22, cfg="random: inplace may_sleep use_finup
>>>> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
>>>> 21.13%@+2728]"
>>>> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
>>>> succeeded on test vector "random: len=1118 klen=32";
>>>> expected_error=-22, cfg="random: may_sleep use_final
>>>> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
>>>>
>>>> These are simply a result of the ECB and CBC implementations not
>>>> returning -EINVAL when the input is not a multiple of the block size.
>>>>
>>>> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
>>>> match generic impl (1)
>>>>
>>>> This means cra_blocksize is not set to 1 as it should. If your driver
>>>> uses the skcipher walk API, it should set the walksize to
>>>> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
>>>> don't, then you can disregard that part.
>>>>
>>>> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
>>>> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
>>>> expected_error=-22
>>>>
>>>> Another missing sanity check. GCM only permits certain authsizes.
>>>>
>>>> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
>>>> unaligned cases.
>>>>
>>>> This is not a bug, but I'm not sure if the below is related or not.
>>>>
>>>> I'll preserve the binaries, in case you need me to objdump anything.
>>>
>>> What are these tests you are executing? For me, the testmgr self test
>>> suite is passing just fine. Any extra tests you have enabled somehow?
>>>
> 
> I enabled CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, which enables a bunch of
> fuzz tests of the offloaded algorithms against the generic
> implementations.

Ahha I see, let me give that a shot locally. I have so far only been 
testing with the standard suite.

> 
>>> I am also running full test on different board though (am57xx), I
>>> haven't been explicitly running anything on am335x.
>>
>> Oh, and btw, did you try without my series? I think the selftests are
>> failing rather miserably without them...
>>
> 
> No, I just tried a branch with mine and your patches applied.

Could you give it a shot without the CRYPTO_MANAGER_EXTRA_TESTS, that 
should pass with my series, and fail without?

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Ard Biesheuvel Oct. 26, 2019, 3:06 p.m. UTC | #6
On Fri, 25 Oct 2019 at 14:18, Tero Kristo <t-kristo@ti.com> wrote:
>
> On 25/10/2019 15:05, Ard Biesheuvel wrote:
> > On Fri, 25 Oct 2019 at 13:56, Tero Kristo <t-kristo@ti.com> wrote:
> >>
> >> On 25/10/2019 14:55, Tero Kristo wrote:
> >>> On 25/10/2019 14:33, Ard Biesheuvel wrote:
> >>>> On Thu, 17 Oct 2019 at 14:26, Tero Kristo <t-kristo@ti.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> This series fixes a number of bugs with omap crypto implementation.
> >>>>> These have become evident with the changes to the cryptomanager, where
> >>>>> it adds some new test cases and modifies some existing, namely the split
> >>>>> update tests. Also, while fixing the cryptomanager induced bugs, some
> >>>>> other surfaced with tcrypt/IPSec tests, so fixed them aswell.
> >>>>>
> >>>>> Patch #9 is against crypto core modifying the crypto_wait_req
> >>>>> common API to have a timeout for it also, currently it waits forever
> >>>>> and it is kind of difficult to see what test fails with crypto manager.
> >>>>> This is not really needed for anything, but it is kind of nice to have
> >>>>> (makes debugging easier.)
> >>>>>
> >>>>> This series has been tested on top of 5.4-rc2, with following setups,
> >>>>> on AM57xx-beagle-x15 board:
> >>>>>
> >>>>> - crypto manager self tests
> >>>>> - tcrypt performance test
> >>>>> - ipsec test with strongswan
> >>>>>
> >>>>> This series depends on the skcipher API switch patch from Ard Biesheuvel
> >>>>> [1].
> >>>>>
> >>>>
> >>>> Hi Tero,
> >>>>
> >>>> On my BeagleBone White, I am hitting the following issues after
> >>>> applying these patches:
> >>>>
> >>>> [    7.493903] alg: skcipher: ecb-aes-omap encryption unexpectedly
> >>>> succeeded on test vector "random: len=531 klen=32";
> >>>> expected_error=-22, cfg="random: inplace may_sleep use_finup
> >>>> src_divs=[44.72%@+4028, <flush>14.70%@alignmask+3, 19.45%@+4070,
> >>>> 21.13%@+2728]"
> >>>> [    7.651103] alg: skcipher: cbc-aes-omap encryption unexpectedly
> >>>> succeeded on test vector "random: len=1118 klen=32";
> >>>> expected_error=-22, cfg="random: may_sleep use_final
> >>>> src_divs=[<reimport>41.87%@+31, <flush>58.13%@+2510]"
> >>>>
> >>>> These are simply a result of the ECB and CBC implementations not
> >>>> returning -EINVAL when the input is not a multiple of the block size.
> >>>>
> >>>> [    7.845527] alg: skcipher: blocksize for ctr-aes-omap (16) doesn't
> >>>> match generic impl (1)
> >>>>
> >>>> This means cra_blocksize is not set to 1 as it should. If your driver
> >>>> uses the skcipher walk API, it should set the walksize to
> >>>> AES_BLOCK_SIZE to ensure that the input is handled correctly. If you
> >>>> don't, then you can disregard that part.
> >>>>
> >>>> [    8.306491] alg: aead: gcm-aes-omap setauthsize unexpectedly
> >>>> succeeded on test vector "random: alen=3 plen=31 authsize=6 klen=9";
> >>>> expected_error=-22
> >>>>
> >>>> Another missing sanity check. GCM only permits certain authsizes.
> >>>>
> >>>> [    9.074703] omap_crypto_copy_sgs: Couldn't allocate pages for
> >>>> unaligned cases.
> >>>>
> >>>> This is not a bug, but I'm not sure if the below is related or not.
> >>>>
> >>>> I'll preserve the binaries, in case you need me to objdump anything.
> >>>
> >>> What are these tests you are executing? For me, the testmgr self test
> >>> suite is passing just fine. Any extra tests you have enabled somehow?
> >>>
> >
> > I enabled CONFIG_CRYPTO_MANAGER_EXTRA_TESTS, which enables a bunch of
> > fuzz tests of the offloaded algorithms against the generic
> > implementations.
>
> Ahha I see, let me give that a shot locally. I have so far only been
> testing with the standard suite.
>
> >
> >>> I am also running full test on different board though (am57xx), I
> >>> haven't been explicitly running anything on am335x.
> >>
> >> Oh, and btw, did you try without my series? I think the selftests are
> >> failing rather miserably without them...
> >>
> >
> > No, I just tried a branch with mine and your patches applied.
>
> Could you give it a shot without the CRYPTO_MANAGER_EXTRA_TESTS, that
> should pass with my series, and fail without?
>

The missing output IVs are fixed by this series, but it seems we need
some more work to get all the wrinkles ironed out. I sent some patches
on top that address a couple of them, but we still need a proper fix
for the situation where only assocdata is presented, and cryptlen == 0

Feel free to merge my patches into your series, or take bits and
pieces into your own patches where needed.