LinuxLists.cc - [PATCH] crypto: vmx - fix copy-paste error in CTR mode

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Hi Daniel,

On Fri, Mar 15, 2019 at 01:09:01PM +1100, Daniel Axtens wrote:
> The original assembly imported from OpenSSL has two copy-paste
> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
> the code branches to the CBC decryption exit path, rather than to
> the CTR exit path.

So does this need to be fixed in OpenSSL too?

>
> This leads to corruption of the IV, which leads to subsequent blocks
> being corrupted.
>
> This can be detected with libkcapi test suite, which is available at
> https://github.com/smuellerDD/libkcapi
>

Is this also detected by the kernel's crypto self-tests, and if not why not?
What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?

> Reported-by: Ondrej Mosnáček <[email protected]>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: [email protected]
> Signed-off-by: Daniel Axtens <[email protected]>
> ---
> drivers/crypto/vmx/aesp8-ppc.pl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/vmx/aesp8-ppc.pl b/drivers/crypto/vmx/aesp8-ppc.pl
> index d6a9f63d65ba..de78282b8f44 100644
> --- a/drivers/crypto/vmx/aesp8-ppc.pl
> +++ b/drivers/crypto/vmx/aesp8-ppc.pl
> @@ -1854,7 +1854,7 @@ Lctr32_enc8x_three:
> stvx_u $out1,$x10,$out
> stvx_u $out2,$x20,$out
> addi $out,$out,0x30
> - b Lcbc_dec8x_done
> + b Lctr32_enc8x_done
>
> .align 5
> Lctr32_enc8x_two:
> @@ -1866,7 +1866,7 @@ Lctr32_enc8x_two:
> stvx_u $out0,$x00,$out
> stvx_u $out1,$x10,$out
> addi $out,$out,0x20
> - b Lcbc_dec8x_done
> + b Lctr32_enc8x_done
>
> .align 5
> Lctr32_enc8x_one:
> --
> 2.19.1
>

2019-03-15 04:24:42

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Hi Eric,

>> The original assembly imported from OpenSSL has two copy-paste
>> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
>> the code branches to the CBC decryption exit path, rather than to
>> the CTR exit path.
>
> So does this need to be fixed in OpenSSL too?

Yes, I'm getting in touch with some people internally (at IBM) about
doing that.

>> This leads to corruption of the IV, which leads to subsequent blocks
>> being corrupted.
>>
>> This can be detected with libkcapi test suite, which is available at
>> https://github.com/smuellerDD/libkcapi
>>
>
> Is this also detected by the kernel's crypto self-tests, and if not why not?
> What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?

It seems the self-tests do not catch it. To catch it, there has to be a
test where the blkcipher_walk creates a walk.nbytes such that
[(the number of AES blocks) mod 8] is either 2 or 3. This happens with
AF_ALG pretty frequently, but when I booted with self-tests it only hit
1, 4, 5, 6 and 7 - it missed 0, 2 and 3.

I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
-next?

Regards,
Daniel

>> Reported-by: Ondrej Mosnáček <[email protected]>
>> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
>> Cc: [email protected]
>> Signed-off-by: Daniel Axtens <[email protected]>
>> ---
>> drivers/crypto/vmx/aesp8-ppc.pl | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/vmx/aesp8-ppc.pl b/drivers/crypto/vmx/aesp8-ppc.pl
>> index d6a9f63d65ba..de78282b8f44 100644
>> --- a/drivers/crypto/vmx/aesp8-ppc.pl
>> +++ b/drivers/crypto/vmx/aesp8-ppc.pl
>> @@ -1854,7 +1854,7 @@ Lctr32_enc8x_three:
>> stvx_u $out1,$x10,$out
>> stvx_u $out2,$x20,$out
>> addi $out,$out,0x30
>> - b Lcbc_dec8x_done
>> + b Lctr32_enc8x_done
>>
>> .align 5
>> Lctr32_enc8x_two:
>> @@ -1866,7 +1866,7 @@ Lctr32_enc8x_two:
>> stvx_u $out0,$x00,$out
>> stvx_u $out1,$x10,$out
>> addi $out,$out,0x20
>> - b Lcbc_dec8x_done
>> + b Lctr32_enc8x_done
>>
>> .align 5
>> Lctr32_enc8x_one:
>> --
>> 2.19.1
>>

2019-03-15 04:34:38

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Hi Daniel,

On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
> Hi Eric,
>
> >> The original assembly imported from OpenSSL has two copy-paste
> >> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
> >> the code branches to the CBC decryption exit path, rather than to
> >> the CTR exit path.
> >
> > So does this need to be fixed in OpenSSL too?
>
> Yes, I'm getting in touch with some people internally (at IBM) about
> doing that.
>
> >> This leads to corruption of the IV, which leads to subsequent blocks
> >> being corrupted.
> >>
> >> This can be detected with libkcapi test suite, which is available at
> >> https://github.com/smuellerDD/libkcapi
> >>
> >
> > Is this also detected by the kernel's crypto self-tests, and if not why not?
> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
>
> It seems the self-tests do not catch it. To catch it, there has to be a
> test where the blkcipher_walk creates a walk.nbytes such that
> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
> AF_ALG pretty frequently, but when I booted with self-tests it only hit
> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
>
> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
> -next?
>
> Regards,
> Daniel

The improvements I recently made to the self-tests are intended to catch exactly
this sort of bug. They were just merged for v5.1, so try the latest mainline.
This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
know), but it may be caught by the regular self-tests now too.

- Eric

2019-03-15 05:23:08

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Eric Biggers <[email protected]> writes:

> Hi Daniel,
>
> On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
>> Hi Eric,
>>
>> >> The original assembly imported from OpenSSL has two copy-paste
>> >> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
>> >> the code branches to the CBC decryption exit path, rather than to
>> >> the CTR exit path.
>> >
>> > So does this need to be fixed in OpenSSL too?
>>
>> Yes, I'm getting in touch with some people internally (at IBM) about
>> doing that.
>>
>> >> This leads to corruption of the IV, which leads to subsequent blocks
>> >> being corrupted.
>> >>
>> >> This can be detected with libkcapi test suite, which is available at
>> >> https://github.com/smuellerDD/libkcapi
>> >>
>> >
>> > Is this also detected by the kernel's crypto self-tests, and if not why not?
>> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
>>
>> It seems the self-tests do not catch it. To catch it, there has to be a
>> test where the blkcipher_walk creates a walk.nbytes such that
>> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
>> AF_ALG pretty frequently, but when I booted with self-tests it only hit
>> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
>>
>> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
>> -next?
>>
>> Regards,
>> Daniel
>
> The improvements I recently made to the self-tests are intended to catch exactly
> this sort of bug. They were just merged for v5.1, so try the latest mainline.
> This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
> know), but it may be caught by the regular self-tests now too.

Well, even the patched code fails with the new self-tests, so clearly
they're catching something! I'll investigate in more detail next week.

Regards,
Daniel

>
> - Eric

2019-03-18 06:03:16

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Daniel Axtens <[email protected]> writes:
> The original assembly imported from OpenSSL has two copy-paste
> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
> the code branches to the CBC decryption exit path, rather than to
> the CTR exit path.
>
> This leads to corruption of the IV, which leads to subsequent blocks
> being corrupted.
>
> This can be detected with libkcapi test suite, which is available at
> https://github.com/smuellerDD/libkcapi
>
> Reported-by: Ondrej Mosnáček <[email protected]>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: [email protected]
> Signed-off-by: Daniel Axtens <[email protected]>

Thanks, this fixes kcapi-enc-test.sh for me.

Tested-by: Michael Ellerman <[email protected]>

cheers

2019-03-18 08:41:51

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Eric Biggers <[email protected]> writes:
> On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
...
>> >> This leads to corruption of the IV, which leads to subsequent blocks
>> >> being corrupted.
>> >>
>> >> This can be detected with libkcapi test suite, which is available at
>> >> https://github.com/smuellerDD/libkcapi
>> >
>> > Is this also detected by the kernel's crypto self-tests, and if not why not?
>> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
>>
>> It seems the self-tests do not catch it. To catch it, there has to be a
>> test where the blkcipher_walk creates a walk.nbytes such that
>> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
>> AF_ALG pretty frequently, but when I booted with self-tests it only hit
>> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
>>
>> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
>> -next?
>
> The improvements I recently made to the self-tests are intended to catch exactly
> this sort of bug. They were just merged for v5.1, so try the latest mainline.
> This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
> know), but it may be caught by the regular self-tests now too.

Enabling the crypto tests (CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n)
actually hides the bug for me.

By which I mean I can't trigger the bug via kcapi-enc-tests.sh, because
the VMX code is never called.

ie:
# zgrep -e CRYPTO_MANAGER -e VMX /proc/config.gz
CONFIG_CRYPTO_MANAGER=y
CONFIG_CRYPTO_MANAGER2=y
# CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
# CONFIG_CRYPTO_MANAGER_EXTRA_TESTS is not set
CONFIG_CRYPTO_DEV_VMX=y
CONFIG_CRYPTO_DEV_VMX_ENCRYPT=y

# echo "p:p8_aes_ctr_crypt p8_aes_ctr_crypt" > /sys/kernel/debug/tracing/kprobe_events
# echo 1 > /sys/kernel/debug/tracing/events/kprobes/enable
# ./kcapi-enc-test.sh
...
Number of failures: 0
# grep -c p8_aes_ctr_crypt /sys/kernel/debug/tracing/trace
0

I don't understand how the crypto core chooses which crypto_alg to use,
but I didn't expect enabling the tests to change it?

cheers

2019-03-18 09:13:52

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On Mon, 18 Mar 2019 at 09:41, Michael Ellerman <[email protected]> wrote:
>
> Eric Biggers <[email protected]> writes:
> > On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
> ...
> >> >> This leads to corruption of the IV, which leads to subsequent blocks
> >> >> being corrupted.
> >> >>
> >> >> This can be detected with libkcapi test suite, which is available at
> >> >> https://github.com/smuellerDD/libkcapi
> >> >
> >> > Is this also detected by the kernel's crypto self-tests, and if not why not?
> >> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
> >>
> >> It seems the self-tests do not catch it. To catch it, there has to be a
> >> test where the blkcipher_walk creates a walk.nbytes such that
> >> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
> >> AF_ALG pretty frequently, but when I booted with self-tests it only hit
> >> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
> >>
> >> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
> >> -next?
> >
> > The improvements I recently made to the self-tests are intended to catch exactly
> > this sort of bug. They were just merged for v5.1, so try the latest mainline.
> > This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
> > know), but it may be caught by the regular self-tests now too.
>
> Enabling the crypto tests (CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n)
> actually hides the bug for me.
>
> By which I mean I can't trigger the bug via kcapi-enc-tests.sh, because
> the VMX code is never called.
>
> ie:
> # zgrep -e CRYPTO_MANAGER -e VMX /proc/config.gz
> CONFIG_CRYPTO_MANAGER=y
> CONFIG_CRYPTO_MANAGER2=y
> # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
> # CONFIG_CRYPTO_MANAGER_EXTRA_TESTS is not set
> CONFIG_CRYPTO_DEV_VMX=y
> CONFIG_CRYPTO_DEV_VMX_ENCRYPT=y
>
> # echo "p:p8_aes_ctr_crypt p8_aes_ctr_crypt" > /sys/kernel/debug/tracing/kprobe_events
> # echo 1 > /sys/kernel/debug/tracing/events/kprobes/enable
> # ./kcapi-enc-test.sh
> ...
> Number of failures: 0
> # grep -c p8_aes_ctr_crypt /sys/kernel/debug/tracing/trace
> 0
>
>
> I don't understand how the crypto core chooses which crypto_alg to use,
> but I didn't expect enabling the tests to change it?
>

This is not entirely unexpected. Based on the tests, algos that are
found to be broken are disregarded for further use, and you should see
a warning in the kernel log about this.

2019-03-19 00:53:02

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Ard Biesheuvel <[email protected]> writes:
> On Mon, 18 Mar 2019 at 09:41, Michael Ellerman <[email protected]> wrote:
...
>>
>> I don't understand how the crypto core chooses which crypto_alg to use,
>> but I didn't expect enabling the tests to change it?
>
> This is not entirely unexpected. Based on the tests, algos that are
> found to be broken are disregarded for further use, and you should see
> a warning in the kernel log about this.

Ah right that makes sense then. I wasn't looking at the kernel log, just
rerunning the kcapi reproducer. Thanks for clarifying.

cheers

2019-03-20 08:41:09

by Ondrej Mosnáček

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Hi Daniel,

pi 15. 3. 2019 o 3:09 Daniel Axtens <[email protected]> napísal(a):
> The original assembly imported from OpenSSL has two copy-paste
> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
> the code branches to the CBC decryption exit path, rather than to
> the CTR exit path.
>
> This leads to corruption of the IV, which leads to subsequent blocks
> being corrupted.
>
> This can be detected with libkcapi test suite, which is available at
> https://github.com/smuellerDD/libkcapi
>
> Reported-by: Ondrej Mosnáček <[email protected]>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: [email protected]
> Signed-off-by: Daniel Axtens <[email protected]>

Thank you for looking into this and for posting the patch(es)! I
tested the patch yesterday and I can confirm that it makes the
libkcapi tests/reproducer pass.

Assuming you will want to cover the other failures from the new
testmgr tests by a separate patch:

Tested-by: Ondrej Mosnacek <[email protected]>

> ---
> drivers/crypto/vmx/aesp8-ppc.pl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/vmx/aesp8-ppc.pl b/drivers/crypto/vmx/aesp8-ppc.pl
> index d6a9f63d65ba..de78282b8f44 100644
> --- a/drivers/crypto/vmx/aesp8-ppc.pl
> +++ b/drivers/crypto/vmx/aesp8-ppc.pl
> @@ -1854,7 +1854,7 @@ Lctr32_enc8x_three:
> stvx_u $out1,$x10,$out
> stvx_u $out2,$x20,$out
> addi $out,$out,0x30
> - b Lcbc_dec8x_done
> + b Lctr32_enc8x_done
>
> .align 5
> Lctr32_enc8x_two:
> @@ -1866,7 +1866,7 @@ Lctr32_enc8x_two:
> stvx_u $out0,$x00,$out
> stvx_u $out1,$x10,$out
> addi $out,$out,0x20
> - b Lcbc_dec8x_done
> + b Lctr32_enc8x_done
>
> .align 5
> Lctr32_enc8x_one:
> --
> 2.19.1
>

2019-03-22 13:04:34

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On Fri, Mar 15, 2019 at 01:09:01PM +1100, Daniel Axtens wrote:
> The original assembly imported from OpenSSL has two copy-paste
> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
> the code branches to the CBC decryption exit path, rather than to
> the CTR exit path.
>
> This leads to corruption of the IV, which leads to subsequent blocks
> being corrupted.
>
> This can be detected with libkcapi test suite, which is available at
> https://github.com/smuellerDD/libkcapi
>
> Reported-by: Ondrej Mosnáček <[email protected]>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: [email protected]
> Signed-off-by: Daniel Axtens <[email protected]>
> ---
> drivers/crypto/vmx/aesp8-ppc.pl | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-04-10 07:02:39

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Hi Daniel,

On Fri, Mar 15, 2019 at 04:23:02PM +1100, Daniel Axtens wrote:
> Eric Biggers <[email protected]> writes:
>
> > Hi Daniel,
> >
> > On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
> >> Hi Eric,
> >>
> >> >> The original assembly imported from OpenSSL has two copy-paste
> >> >> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
> >> >> the code branches to the CBC decryption exit path, rather than to
> >> >> the CTR exit path.
> >> >
> >> > So does this need to be fixed in OpenSSL too?
> >>
> >> Yes, I'm getting in touch with some people internally (at IBM) about
> >> doing that.
> >>
> >> >> This leads to corruption of the IV, which leads to subsequent blocks
> >> >> being corrupted.
> >> >>
> >> >> This can be detected with libkcapi test suite, which is available at
> >> >> https://github.com/smuellerDD/libkcapi
> >> >>
> >> >
> >> > Is this also detected by the kernel's crypto self-tests, and if not why not?
> >> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
> >>
> >> It seems the self-tests do not catch it. To catch it, there has to be a
> >> test where the blkcipher_walk creates a walk.nbytes such that
> >> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
> >> AF_ALG pretty frequently, but when I booted with self-tests it only hit
> >> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
> >>
> >> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
> >> -next?
> >>
> >> Regards,
> >> Daniel
> >
> > The improvements I recently made to the self-tests are intended to catch exactly
> > this sort of bug. They were just merged for v5.1, so try the latest mainline.
> > This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
> > know), but it may be caught by the regular self-tests now too.
>
> Well, even the patched code fails with the new self-tests, so clearly
> they're catching something! I'll investigate in more detail next week.
>
> Regards,
> Daniel
>
> >
> > - Eric

Are you still planning to fix the remaining bug? I booted a ppc64le VM, and I
see the same test failure (I think) you were referring to:

alg: skcipher: p8_aes_ctr encryption test failed (wrong result) on test vector 3, cfg="uneven misaligned splits, may sleep"

- Eric

2019-04-11 14:47:31

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Eric Biggers <[email protected]> writes:

> Hi Daniel,
>
> On Fri, Mar 15, 2019 at 04:23:02PM +1100, Daniel Axtens wrote:
>> Eric Biggers <[email protected]> writes:
>>
>> > Hi Daniel,
>> >
>> > On Fri, Mar 15, 2019 at 03:24:35PM +1100, Daniel Axtens wrote:
>> >> Hi Eric,
>> >>
>> >> >> The original assembly imported from OpenSSL has two copy-paste
>> >> >> errors in handling CTR mode. When dealing with a 2 or 3 block tail,
>> >> >> the code branches to the CBC decryption exit path, rather than to
>> >> >> the CTR exit path.
>> >> >
>> >> > So does this need to be fixed in OpenSSL too?
>> >>
>> >> Yes, I'm getting in touch with some people internally (at IBM) about
>> >> doing that.
>> >>
>> >> >> This leads to corruption of the IV, which leads to subsequent blocks
>> >> >> being corrupted.
>> >> >>
>> >> >> This can be detected with libkcapi test suite, which is available at
>> >> >> https://github.com/smuellerDD/libkcapi
>> >> >>
>> >> >
>> >> > Is this also detected by the kernel's crypto self-tests, and if not why not?
>> >> > What about with the new option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y?
>> >>
>> >> It seems the self-tests do not catch it. To catch it, there has to be a
>> >> test where the blkcipher_walk creates a walk.nbytes such that
>> >> [(the number of AES blocks) mod 8] is either 2 or 3. This happens with
>> >> AF_ALG pretty frequently, but when I booted with self-tests it only hit
>> >> 1, 4, 5, 6 and 7 - it missed 0, 2 and 3.
>> >>
>> >> I don't have the EXTRA_TESTS option - I'm testing with 5.0-rc6. Is it in
>> >> -next?
>> >>
>> >> Regards,
>> >> Daniel
>> >
>> > The improvements I recently made to the self-tests are intended to catch exactly
>> > this sort of bug. They were just merged for v5.1, so try the latest mainline.
>> > This almost certainly would be caught by EXTRA_TESTS (and if not I'd want to
>> > know), but it may be caught by the regular self-tests now too.
>>
>> Well, even the patched code fails with the new self-tests, so clearly
>> they're catching something! I'll investigate in more detail next week.
>>
>> Regards,
>> Daniel
>>
>> >
>> > - Eric

Hi Eric,

>
> Are you still planning to fix the remaining bug? I booted a ppc64le VM, and I
> see the same test failure (I think) you were referring to:
>
> alg: skcipher: p8_aes_ctr encryption test failed (wrong result) on test vector 3, cfg="uneven misaligned splits, may sleep"
>

Yes, that's the one I saw. I don't have time to follow it up at the
moment, but Nayna is aware of it.

Regards,
Daniel

> - Eric

2019-04-11 17:41:01

by Nayna Jain

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On 04/11/2019 10:47 AM, Daniel Axtens wrote:
> Eric Biggers <[email protected]> writes:
>
>> Are you still planning to fix the remaining bug? I booted a ppc64le VM, and I
>> see the same test failure (I think) you were referring to:
>>
>> alg: skcipher: p8_aes_ctr encryption test failed (wrong result) on test vector 3, cfg="uneven misaligned splits, may sleep"
>>
> Yes, that's the one I saw. I don't have time to follow it up at the
> moment, but Nayna is aware of it.
>

Yes Eric, we identified this as a separate issue of misalignment and
plan to post a separate patch to address it.

Thanks & Regards,
- Nayna

2019-04-13 03:41:41

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Nayna <[email protected]> writes:

> On 04/11/2019 10:47 AM, Daniel Axtens wrote:
>> Eric Biggers <[email protected]> writes:
>>
>>> Are you still planning to fix the remaining bug? I booted a ppc64le VM, and I
>>> see the same test failure (I think) you were referring to:
>>>
>>> alg: skcipher: p8_aes_ctr encryption test failed (wrong result) on test vector 3, cfg="uneven misaligned splits, may sleep"
>>>
>> Yes, that's the one I saw. I don't have time to follow it up at the
>> moment, but Nayna is aware of it.
>>
>
> Yes Eric, we identified this as a separate issue of misalignment and
> plan to post a separate patch to address it.

I also wrote it down in my write-only TODO list here:

https://github.com/linuxppc/issues/issues/238

cheers

2019-05-06 15:53:43

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On Sat, Apr 13, 2019 at 01:41:36PM +1000, Michael Ellerman wrote:
> Nayna <[email protected]> writes:
>
> > On 04/11/2019 10:47 AM, Daniel Axtens wrote:
> >> Eric Biggers <[email protected]> writes:
> >>
> >>> Are you still planning to fix the remaining bug? I booted a ppc64le VM, and I
> >>> see the same test failure (I think) you were referring to:
> >>>
> >>> alg: skcipher: p8_aes_ctr encryption test failed (wrong result) on test vector 3, cfg="uneven misaligned splits, may sleep"
> >>>
> >> Yes, that's the one I saw. I don't have time to follow it up at the
> >> moment, but Nayna is aware of it.
> >>
> >
> > Yes Eric, we identified this as a separate issue of misalignment and
> > plan to post a separate patch to address it.
>
> I also wrote it down in my write-only TODO list here:
>
> https://github.com/linuxppc/issues/issues/238
>
>
> cheers

Any progress on this? Someone just reported this again here:
https://bugzilla.kernel.org/show_bug.cgi?id=203515

- Eric

2019-05-13 01:12:21

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On Mon, May 06, 2019 at 08:53:17AM -0700, Eric Biggers wrote:
>
> Any progress on this? Someone just reported this again here:
> https://bugzilla.kernel.org/show_bug.cgi?id=203515

Guys if I don't get a fix for this soon I'll have to disable CTR
in vmx.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-05-13 12:41:40

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Herbert Xu <[email protected]> writes:
> On Mon, May 06, 2019 at 08:53:17AM -0700, Eric Biggers wrote:
>>
>> Any progress on this? Someone just reported this again here:
>> https://bugzilla.kernel.org/show_bug.cgi?id=203515
>
> Guys if I don't get a fix for this soon I'll have to disable CTR
> in vmx.

No objection from me.

I'll try and debug it at some point if no one else does, but I can't
make it my top priority sorry.

cheers

2019-05-14 17:36:17

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Michael Ellerman <[email protected]> writes:

> Herbert Xu <[email protected]> writes:
>> On Mon, May 06, 2019 at 08:53:17AM -0700, Eric Biggers wrote:
>>>
>>> Any progress on this? Someone just reported this again here:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=203515
>>
>> Guys if I don't get a fix for this soon I'll have to disable CTR
>> in vmx.
>
> No objection from me.
>
> I'll try and debug it at some point if no one else does, but I can't
> make it my top priority sorry.

I'm a bit concerned that this will end up filtering down to distros and
tanking crypto performance for the entire lifespan of the releases, so
I'd rather fix it if I can.

A quick additional test reveals an issue in the uneven misaligned
splits. (the may-sleep may reveal an extra bug, but there's at least one
with uneven/misaligned.)

By all means disable vmx ctr if I don't get an answer to you in a
timeframe you are comfortable with, but I am going to at least try to
have a look.

Regards,
Daniel

>
> cheers

2019-05-15 03:54:49

by Herbert Xu

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On Wed, May 15, 2019 at 03:35:51AM +1000, Daniel Axtens wrote:
>
> By all means disable vmx ctr if I don't get an answer to you in a
> timeframe you are comfortable with, but I am going to at least try to
> have a look.

I'm happy to give you guys more time. How much time do you think
you will need?

Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-05-15 06:36:43

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Herbert Xu <[email protected]> writes:

> On Wed, May 15, 2019 at 03:35:51AM +1000, Daniel Axtens wrote:
>>
>> By all means disable vmx ctr if I don't get an answer to you in a
>> timeframe you are comfortable with, but I am going to at least try to
>> have a look.
>
> I'm happy to give you guys more time. How much time do you think
> you will need?
>
Give me till the end of the week: if I haven't solved it by then I will
probably have to give up and go on to other things anyway.

(FWIW, it seems to happen when encoding greater than 4 but less than 8
AES blocks - in particular with both 7 and 5 blocks encoded I can see it
go wrong from block 4 onwards. No idea why yet, and the asm is pretty
dense, but that's where I'm at at the moment.)

Regards,
Daniel

> Thanks,
> --
> Email: Herbert Xu <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-05-16 02:13:53

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

Daniel Axtens <[email protected]> writes:

> Herbert Xu <[email protected]> writes:
>
>> On Wed, May 15, 2019 at 03:35:51AM +1000, Daniel Axtens wrote:
>>>
>>> By all means disable vmx ctr if I don't get an answer to you in a
>>> timeframe you are comfortable with, but I am going to at least try to
>>> have a look.
>>
>> I'm happy to give you guys more time. How much time do you think
>> you will need?
>>
> Give me till the end of the week: if I haven't solved it by then I will
> probably have to give up and go on to other things anyway.

So as you've hopefully seen, I've nailed it down and posted a patch.
(http://patchwork.ozlabs.org/patch/1099934/)

I'm also seeing issues with ghash with the extended tests:

[ 7.582926] alg: hash: p8_ghash test failed (wrong result) on test vector 0, cfg="random: use_final src_divs=[<reimport>9.72%@+39832, <reimport>18.2%@+65504, <reimport,nosimd>45.57%@alignmask+18, <reimport,nosimd>15.6%@+65496, 6.83%@+65514, <reimport,nosimd>1.2%@+25, <reim"

It seems to happen when one of the source divisions has nosimd and the
final result uses the simd finaliser, so that's interesting.

Regards,
Daniel

>
> (FWIW, it seems to happen when encoding greater than 4 but less than 8
> AES blocks - in particular with both 7 and 5 blocks encoded I can see it
> go wrong from block 4 onwards. No idea why yet, and the asm is pretty
> dense, but that's where I'm at at the moment.)
>
> Regards,
> Daniel
>
>> Thanks,
>> --
>> Email: Herbert Xu <[email protected]>
>> Home Page: http://gondor.apana.org.au/~herbert/
>> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-05-16 02:57:16

[permalink] [raw]

Subject: Re: [PATCH] crypto: vmx - fix copy-paste error in CTR mode

On Thu, May 16, 2019 at 12:12:48PM +1000, Daniel Axtens wrote:
>
> I'm also seeing issues with ghash with the extended tests:
>
> [ 7.582926] alg: hash: p8_ghash test failed (wrong result) on test vector 0, cfg="random: use_final src_divs=[<reimport>9.72%@+39832, <reimport>18.2%@+65504, <reimport,nosimd>45.57%@alignmask+18, <reimport,nosimd>15.6%@+65496, 6.83%@+65514, <reimport,nosimd>1.2%@+25, <reim"
>
> It seems to happen when one of the source divisions has nosimd and the
> final result uses the simd finaliser, so that's interesting.
>

The bug is that p8_ghash uses different shash_descs for the SIMD and no-SIMD
cases. So if you start out doing the hash in SIMD context but then switch to
no-SIMD context or vice versa, the digest will be wrong. Note that there can be
an ->export() and ->import() in between, so it's not quite as obscure a case as
one might think.

To fix it I think you'll need to make p8_ghash use 'struct ghash_desc_ctx' just
like ghash-generic so that the two code paths can share the same shash_desc.
That's similar to what the various SHA hash algorithms do.

- Eric

2019-05-16 05:30:11