| Message ID | 20190315020901.16509-1-dja@axtens.net (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: vmx - fix copy-paste error in CTR mode | expand |
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 >
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 >>
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
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
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
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
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.
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
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 >
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.
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
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
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
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
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
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,
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
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
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,
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
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
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
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
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:
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(-)