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

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

[Daniel Axtens]

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 <omosnacek@gmail.com>
Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Axtens <dja@axtens.net>
---
 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

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

[Eric Biggers]

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 <omosnacek@gmail.com>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  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
> 

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

[Daniel Axtens]

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 <omosnacek@gmail.com>
>> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> ---
>>  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
>> 

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

[Eric Biggers]

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

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

[Daniel Axtens]

Eric Biggers <ebiggers@kernel.org> 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

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

[Michael Ellerman]

Daniel Axtens <dja@axtens.net> 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 <omosnacek@gmail.com>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Axtens <dja@axtens.net>

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

Tested-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

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

[Michael Ellerman]

Eric Biggers <ebiggers@kernel.org> 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

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

[Ard Biesheuvel]

On Mon, 18 Mar 2019 at 09:41, Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Eric Biggers <ebiggers@kernel.org> 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.

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

[Michael Ellerman]

Ard Biesheuvel <ard.biesheuvel@linaro.org> writes:
> On Mon, 18 Mar 2019 at 09:41, Michael Ellerman <mpe@ellerman.id.au> 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

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

[Ondrej Mosnáček]

Hi Daniel,

pi 15. 3. 2019 o 3:09 Daniel Axtens <dja@axtens.net> 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 <omosnacek@gmail.com>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Axtens <dja@axtens.net>

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 <omosnacek@gmail.com>

> ---
>  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
>

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

[Herbert Xu]

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 <omosnacek@gmail.com>
> Fixes: 5c380d623ed3 ("crypto: vmx - Add support for VMS instructions by ASM")
> Cc: stable@vger.kernel.org
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  drivers/crypto/vmx/aesp8-ppc.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Eric Biggers]

Hi Daniel,

On Fri, Mar 15, 2019 at 04:23:02PM +1100, Daniel Axtens wrote:
> Eric Biggers <ebiggers@kernel.org> 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

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

[Daniel Axtens]

Eric Biggers <ebiggers@kernel.org> writes:

> Hi Daniel,
>
> On Fri, Mar 15, 2019 at 04:23:02PM +1100, Daniel Axtens wrote:
>> Eric Biggers <ebiggers@kernel.org> 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

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

[Nayna]

On 04/11/2019 10:47 AM, Daniel Axtens wrote:
> Eric Biggers <ebiggers@kernel.org> 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

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

[Michael Ellerman]

Nayna <nayna@linux.vnet.ibm.com> writes:

> On 04/11/2019 10:47 AM, Daniel Axtens wrote:
>> Eric Biggers <ebiggers@kernel.org> 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

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

[Eric Biggers]

On Sat, Apr 13, 2019 at 01:41:36PM +1000, Michael Ellerman wrote:
> Nayna <nayna@linux.vnet.ibm.com> writes:
> 
> > On 04/11/2019 10:47 AM, Daniel Axtens wrote:
> >> Eric Biggers <ebiggers@kernel.org> 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

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

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Michael Ellerman]

Herbert Xu <herbert@gondor.apana.org.au> 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

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

[Daniel Axtens]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Herbert Xu <herbert@gondor.apana.org.au> 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

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

[Herbert Xu]

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Daniel Axtens]

Herbert Xu <herbert@gondor.apana.org.au> 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 <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Daniel Axtens]

Daniel Axtens <dja@axtens.net> writes:

> Herbert Xu <herbert@gondor.apana.org.au> 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 <herbert@gondor.apana.org.au>
>> Home Page: http://gondor.apana.org.au/~herbert/
>> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Eric Biggers]

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

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

[Daniel Axtens]

Eric Biggers <ebiggers@kernel.org> writes:

> 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.

Ah cool, I was just in the process of figuring this out for myself -
always lovely to have my theory confirmed!

> 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.

This is very helpful, thank you. I guess I will do that then.

Regards,
Daniel

>
> - Eric