linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks
@ 2020-03-18 23:45 Jason A. Donenfeld
  2020-03-19  0:23 ` Eric Biggers
  0 siblings, 1 reply; 11+ messages in thread
From: Jason A. Donenfeld @ 2020-03-18 23:45 UTC (permalink / raw)
  To: linux-kernel, linux-crypto, gregkh, herbert
  Cc: Jason A. Donenfeld, Emil Renner Berthing, Ard Biesheuvel, stable

Prior, passing in chunks of 2, 3, or 4, followed by any additional
chunks would result in the chacha state counter getting out of sync,
resulting in incorrect encryption/decryption, which is a pretty nasty
crypto vuln, dating back to 2018. WireGuard users never experienced this
prior, because we have always, out of tree, used a different crypto
library, until the recent Frankenzinc addition. This commit fixes the
issue by advancing the pointers and state counter by the actual size
processed.

Fixes: f2ca1cbd0fb5 ("crypto: arm64/chacha - optimize for arbitrary length inputs")
Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/crypto/chacha-neon-glue.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c
index c1f9660d104c..debb1de0d3dd 100644
--- a/arch/arm64/crypto/chacha-neon-glue.c
+++ b/arch/arm64/crypto/chacha-neon-glue.c
@@ -55,10 +55,10 @@ static void chacha_doneon(u32 *state, u8 *dst, const u8 *src,
 			break;
 		}
 		chacha_4block_xor_neon(state, dst, src, nrounds, l);
-		bytes -= CHACHA_BLOCK_SIZE * 5;
-		src += CHACHA_BLOCK_SIZE * 5;
-		dst += CHACHA_BLOCK_SIZE * 5;
-		state[12] += 5;
+		bytes -= l;
+		src += l;
+		dst += l;
+		state[12] += round_up(l, CHACHA_BLOCK_SIZE) / CHACHA_BLOCK_SIZE;
 	}
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks
  2020-03-18 23:45 [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks Jason A. Donenfeld
@ 2020-03-19  0:23 ` Eric Biggers
  2020-03-19  0:30   ` Eric Biggers
  2020-03-19  1:33   ` Jason A. Donenfeld
  0 siblings, 2 replies; 11+ messages in thread
From: Eric Biggers @ 2020-03-19  0:23 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, gregkh, herbert,
	Emil Renner Berthing, Ard Biesheuvel, stable

Hi Jason,

On Wed, Mar 18, 2020 at 05:45:18PM -0600, Jason A. Donenfeld wrote:
> Prior, passing in chunks of 2, 3, or 4, followed by any additional
> chunks would result in the chacha state counter getting out of sync,
> resulting in incorrect encryption/decryption, which is a pretty nasty
> crypto vuln, dating back to 2018. WireGuard users never experienced this
> prior, because we have always, out of tree, used a different crypto
> library, until the recent Frankenzinc addition. This commit fixes the
> issue by advancing the pointers and state counter by the actual size
> processed.
> 
> Fixes: f2ca1cbd0fb5 ("crypto: arm64/chacha - optimize for arbitrary length inputs")
> Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: stable@vger.kernel.org

Thanks for fixing this!  We definitely should get this fix to Linus for 5.6.
But I don't think your description of this bug dating back to 2018 is accurate,
because this bug only affects the new library interface to ChaCha20 which was
added in v5.5.  In the "regular" crypto API case, the "walksize" is set to
'5 * CHACHA_BLOCK_SIZE', and chacha_doneon() is guaranteed to be called with a
multiple of '5 * CHACHA_BLOCK_SIZE' except at the end.  Thus the code worked
fine with the regular crypto API.

In fact we have fuzz tests for the regular crypto API which find bugs exactly
like these.  For example, they try dividing the data up randomly into chunks.
It would be great if the new library interface had fuzz tests too.

> diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c
> index c1f9660d104c..debb1de0d3dd 100644
> --- a/arch/arm64/crypto/chacha-neon-glue.c
> +++ b/arch/arm64/crypto/chacha-neon-glue.c
> @@ -55,10 +55,10 @@ static void chacha_doneon(u32 *state, u8 *dst, const u8 *src,
>  			break;
>  		}
>  		chacha_4block_xor_neon(state, dst, src, nrounds, l);
> -		bytes -= CHACHA_BLOCK_SIZE * 5;
> -		src += CHACHA_BLOCK_SIZE * 5;
> -		dst += CHACHA_BLOCK_SIZE * 5;
> -		state[12] += 5;
> +		bytes -= l;
> +		src += l;
> +		dst += l;
> +		state[12] += round_up(l, CHACHA_BLOCK_SIZE) / CHACHA_BLOCK_SIZE;

Use DIV_ROUND_UP(l, CHACHA_BLOCK_SIZE)?

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  0:23 ` Eric Biggers
@ 2020-03-19  0:30   ` Eric Biggers
  2020-03-19  1:33   ` Jason A. Donenfeld
  1 sibling, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2020-03-19  0:30 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, gregkh, herbert,
	Emil Renner Berthing, Ard Biesheuvel, stable

On Wed, Mar 18, 2020 at 05:24:01PM -0700, Eric Biggers wrote:
> Hi Jason,
> 
> On Wed, Mar 18, 2020 at 05:45:18PM -0600, Jason A. Donenfeld wrote:
> > Prior, passing in chunks of 2, 3, or 4, followed by any additional
> > chunks would result in the chacha state counter getting out of sync,
> > resulting in incorrect encryption/decryption, which is a pretty nasty
> > crypto vuln, dating back to 2018. WireGuard users never experienced this
> > prior, because we have always, out of tree, used a different crypto
> > library, until the recent Frankenzinc addition. This commit fixes the
> > issue by advancing the pointers and state counter by the actual size
> > processed.
> > 
> > Fixes: f2ca1cbd0fb5 ("crypto: arm64/chacha - optimize for arbitrary length inputs")
> > Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: stable@vger.kernel.org
> 
> Thanks for fixing this!  We definitely should get this fix to Linus for 5.6.
> But I don't think your description of this bug dating back to 2018 is accurate,
> because this bug only affects the new library interface to ChaCha20 which was
> added in v5.5.  In the "regular" crypto API case, the "walksize" is set to
> '5 * CHACHA_BLOCK_SIZE', and chacha_doneon() is guaranteed to be called with a
> multiple of '5 * CHACHA_BLOCK_SIZE' except at the end.  Thus the code worked
> fine with the regular crypto API.

So I think it's actually:

Fixes: b3aad5bad26a ("crypto: arm64/chacha - expose arm64 ChaCha routine as library function")
Cc: <stable@vger.kernel.org> # v5.5+

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  0:23 ` Eric Biggers
  2020-03-19  0:30   ` Eric Biggers
@ 2020-03-19  1:33   ` Jason A. Donenfeld
  2020-03-19  2:27     ` [PATCH URGENT crypto v2] " Jason A. Donenfeld
  1 sibling, 1 reply; 11+ messages in thread
From: Jason A. Donenfeld @ 2020-03-19  1:33 UTC (permalink / raw)
  To: Eric Biggers
  Cc: LKML, Linux Crypto Mailing List, Greg Kroah-Hartman, Herbert Xu,
	Emil Renner Berthing, Ard Biesheuvel, stable

Hey Eric,

On Wed, Mar 18, 2020 at 6:24 PM Eric Biggers <ebiggers@kernel.org> wrote:
> Thanks for fixing this!  We definitely should get this fix to Linus for 5.6.
> But I don't think your description of this bug dating back to 2018 is accurate,
> because this bug only affects the new library interface to ChaCha20 which was
> added in v5.5.  In the "regular" crypto API case, the "walksize" is set to
> '5 * CHACHA_BLOCK_SIZE', and chacha_doneon() is guaranteed to be called with a
> multiple of '5 * CHACHA_BLOCK_SIZE' except at the end.  Thus the code worked
> fine with the regular crypto API.

Ahhh, that seems correct.

> > +             state[12] += round_up(l, CHACHA_BLOCK_SIZE) / CHACHA_BLOCK_SIZE;
>
> Use DIV_ROUND_UP(l, CHACHA_BLOCK_SIZE)?

Duh, oops, thanks. Will send a v2 in a few minutes.

By the way, I took a brief look at the other implementations
accessible from lib/crypto and I didn't see the same issue over there.
But I wouldn't mind an extra pair of eyes, if you want to give it a
quick look too.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  1:33   ` Jason A. Donenfeld
@ 2020-03-19  2:27     ` Jason A. Donenfeld
  2020-03-19  3:25       ` Eric Biggers
                         ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jason A. Donenfeld @ 2020-03-19  2:27 UTC (permalink / raw)
  To: linux-kernel, linux-crypto, gregkh, herbert
  Cc: Jason A. Donenfeld, Emil Renner Berthing, Ard Biesheuvel, stable

Prior, passing in chunks of 2, 3, or 4, followed by any additional
chunks would result in the chacha state counter getting out of sync,
resulting in incorrect encryption/decryption, which is a pretty nasty
crypto vuln: "why do images look weird on webpages?" WireGuard users
never experienced this prior, because we have always, out of tree, used
a different crypto library, until the recent Frankenzinc addition. This
commit fixes the issue by advancing the pointers and state counter by
the actual size processed. It also fixes up a bug in the (optional,
costly) stride test that prevented it from running on arm64.

Fixes: b3aad5bad26a ("crypto: arm64/chacha - expose arm64 ChaCha routine as library function")
Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: stable@vger.kernel.org # v5.5+
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/arm64/crypto/chacha-neon-glue.c   |  8 ++++----
 lib/crypto/chacha20poly1305-selftest.c | 11 ++++++++---
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c
index c1f9660d104c..37ca3e889848 100644
--- a/arch/arm64/crypto/chacha-neon-glue.c
+++ b/arch/arm64/crypto/chacha-neon-glue.c
@@ -55,10 +55,10 @@ static void chacha_doneon(u32 *state, u8 *dst, const u8 *src,
 			break;
 		}
 		chacha_4block_xor_neon(state, dst, src, nrounds, l);
-		bytes -= CHACHA_BLOCK_SIZE * 5;
-		src += CHACHA_BLOCK_SIZE * 5;
-		dst += CHACHA_BLOCK_SIZE * 5;
-		state[12] += 5;
+		bytes -= l;
+		src += l;
+		dst += l;
+		state[12] += DIV_ROUND_UP(l, CHACHA_BLOCK_SIZE);
 	}
 }
 
diff --git a/lib/crypto/chacha20poly1305-selftest.c b/lib/crypto/chacha20poly1305-selftest.c
index c391a91364e9..fa43deda2660 100644
--- a/lib/crypto/chacha20poly1305-selftest.c
+++ b/lib/crypto/chacha20poly1305-selftest.c
@@ -9028,10 +9028,15 @@ bool __init chacha20poly1305_selftest(void)
 	     && total_len <= 1 << 10; ++total_len) {
 		for (i = 0; i <= total_len; ++i) {
 			for (j = i; j <= total_len; ++j) {
+				k = 0;
 				sg_init_table(sg_src, 3);
-				sg_set_buf(&sg_src[0], input, i);
-				sg_set_buf(&sg_src[1], input + i, j - i);
-				sg_set_buf(&sg_src[2], input + j, total_len - j);
+				if (i)
+					sg_set_buf(&sg_src[k++], input, i);
+				if (j - i)
+					sg_set_buf(&sg_src[k++], input + i, j - i);
+				if (total_len - j)
+					sg_set_buf(&sg_src[k++], input + j, total_len - j);
+				sg_init_marker(sg_src, k);
 				memset(computed_output, 0, total_len);
 				memset(input, 0, total_len);
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  2:27     ` [PATCH URGENT crypto v2] " Jason A. Donenfeld
@ 2020-03-19  3:25       ` Eric Biggers
  2020-03-19  4:25         ` Jason A. Donenfeld
  2020-03-19 19:03       ` Eric Biggers
  2020-03-20  3:48       ` Herbert Xu
  2 siblings, 1 reply; 11+ messages in thread
From: Eric Biggers @ 2020-03-19  3:25 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, gregkh, herbert,
	Emil Renner Berthing, Ard Biesheuvel, stable

On Wed, Mar 18, 2020 at 08:27:32PM -0600, Jason A. Donenfeld wrote:
> It also fixes up a bug in the (optional, costly) stride test that
> prevented it from running on arm64.
[...]
> diff --git a/lib/crypto/chacha20poly1305-selftest.c b/lib/crypto/chacha20poly1305-selftest.c
> index c391a91364e9..fa43deda2660 100644
> --- a/lib/crypto/chacha20poly1305-selftest.c
> +++ b/lib/crypto/chacha20poly1305-selftest.c
> @@ -9028,10 +9028,15 @@ bool __init chacha20poly1305_selftest(void)
>  	     && total_len <= 1 << 10; ++total_len) {
>  		for (i = 0; i <= total_len; ++i) {
>  			for (j = i; j <= total_len; ++j) {
> +				k = 0;
>  				sg_init_table(sg_src, 3);
> -				sg_set_buf(&sg_src[0], input, i);
> -				sg_set_buf(&sg_src[1], input + i, j - i);
> -				sg_set_buf(&sg_src[2], input + j, total_len - j);
> +				if (i)
> +					sg_set_buf(&sg_src[k++], input, i);
> +				if (j - i)
> +					sg_set_buf(&sg_src[k++], input + i, j - i);
> +				if (total_len - j)
> +					sg_set_buf(&sg_src[k++], input + j, total_len - j);
> +				sg_init_marker(sg_src, k);
>  				memset(computed_output, 0, total_len);
>  				memset(input, 0, total_len);

So with this test fix, does this test find the bug?

Apparently the empty scatterlist elements caused some problem?  What was
that problem exactly?  And what do you mean by this "prevented the test
from running on arm64"?  If there is a problem it seems we should
something else about about it, e.g. debug checks that work in consistent
way on all architectures, documenting what the function expects, or make
it just work properly with empty scatterlist elements.

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  3:25       ` Eric Biggers
@ 2020-03-19  4:25         ` Jason A. Donenfeld
  2020-03-19  4:36           ` Jason A. Donenfeld
  0 siblings, 1 reply; 11+ messages in thread
From: Jason A. Donenfeld @ 2020-03-19  4:25 UTC (permalink / raw)
  To: Eric Biggers
  Cc: LKML, Linux Crypto Mailing List, Greg Kroah-Hartman, Herbert Xu,
	Emil Renner Berthing, Ard Biesheuvel, stable

On Wed, Mar 18, 2020 at 9:25 PM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Mar 18, 2020 at 08:27:32PM -0600, Jason A. Donenfeld wrote:
> > It also fixes up a bug in the (optional, costly) stride test that
> > prevented it from running on arm64.
> [...]
> > diff --git a/lib/crypto/chacha20poly1305-selftest.c b/lib/crypto/chacha20poly1305-selftest.c
> > index c391a91364e9..fa43deda2660 100644
> > --- a/lib/crypto/chacha20poly1305-selftest.c
> > +++ b/lib/crypto/chacha20poly1305-selftest.c
> > @@ -9028,10 +9028,15 @@ bool __init chacha20poly1305_selftest(void)
> >            && total_len <= 1 << 10; ++total_len) {
> >               for (i = 0; i <= total_len; ++i) {
> >                       for (j = i; j <= total_len; ++j) {
> > +                             k = 0;
> >                               sg_init_table(sg_src, 3);
> > -                             sg_set_buf(&sg_src[0], input, i);
> > -                             sg_set_buf(&sg_src[1], input + i, j - i);
> > -                             sg_set_buf(&sg_src[2], input + j, total_len - j);
> > +                             if (i)
> > +                                     sg_set_buf(&sg_src[k++], input, i);
> > +                             if (j - i)
> > +                                     sg_set_buf(&sg_src[k++], input + i, j - i);
> > +                             if (total_len - j)
> > +                                     sg_set_buf(&sg_src[k++], input + j, total_len - j);
> > +                             sg_init_marker(sg_src, k);
> >                               memset(computed_output, 0, total_len);
> >                               memset(input, 0, total_len);
>
> So with this test fix, does this test find the bug?

Yes.

> Apparently the empty scatterlist elements caused some problem?  What was
> that problem exactly?  And what do you mean by this "prevented the test
> from running on arm64"?  If there is a problem it seems we should
> something else about about it, e.g. debug checks that work in consistent
> way on all architectures, documenting what the function expects, or make
> it just work properly with empty scatterlist elements.

Yea, my next plan was to look into what's going on there. In case
you're curious, here's what happens:

[    0.355761] [ffffffff041e9508] pgd=000000004ffe9003,
pud=000000004ffe9003, pmd=0000000000000000
[    0.356212] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[    0.356587] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc5+ #10
[    0.356695] Hardware name: linux,dummy-virt (DT)
[    0.356972] pstate: 20000005 (nzCv daif -PAN -UAO)
[    0.357158] pc : scatterwalk_copychunks+0x13c/0x1e0
[    0.357257] lr : scatterwalk_copychunks+0xe0/0x1e0
[    0.357379] sp : ffffff800f82f970
[    0.357450] x29: ffffff800f82f970 x28: ffffff800f838000
[    0.357607] x27: ffffff800f838000 x26: 0000000000000001
[    0.357725] x25: ffffff800f82faf0 x24: ffffff800f82fa10
[    0.357820] x23: 0000000000000010 x22: 0000000000000000
[    0.357898] x21: 0000000000000000 x20: 0000000100200000
[    0.357978] x19: ffffff8000000000 x18: 0000000000000014
[    0.358132] x17: 0000000000000000 x16: 0000000000000002
[    0.358214] x15: 00000000074c5755 x14: 0000000000000002
[    0.358310] x13: 074c57550311ac67 x12: 09579e470077af1a
[    0.358453] x11: 1d3f98018ec6f5bb x10: 3e6144a223343e11
[    0.358560] x9 : 0000000000000000 x8 : ffffff800f82fcc0
[    0.358638] x7 : 0000000000000000 x6 : ffffff800fa55000
[    0.358711] x5 : 0000000000001000 x4 : 0000000000000000
[    0.358781] x3 : ffffffff001e9540 x2 : ffffffff001e9540
[    0.358855] x1 : ffffffff041e9500 x0 : ffffff800f82fd60
[    0.359007] Call trace:
[    0.359177]  scatterwalk_copychunks+0x13c/0x1e0
[    0.359269]  scatterwalk_map_and_copy+0x50/0xa8
[    0.359339]  chacha20poly1305_crypt_sg_inplace+0x3f4/0x418
[    0.359418]  chacha20poly1305_encrypt_sg_inplace+0x10/0x18
[    0.359511]  chacha20poly1305_selftest+0x4dc/0x618
[    0.359581]  mod_init+0xc/0x2c
[    0.359630]  do_one_initcall+0x58/0x11c
[    0.359687]  kernel_init_freeable+0x174/0x1e0
[    0.359751]  kernel_init+0x10/0x100
[    0.359803]  ret_from_fork+0x10/0x18
[    0.360074] Code: f9400002 530c7c21 927ef442 8b011841 (f9400423)
[    0.360257] ---[ end trace 0b64264f8a25dbdf ]---
[    0.360453] Kernel panic - not syncing: Fatal exception
[    0.360597] SMP: stopping secondary CPUs
[    0.360835] Kernel Offset: disabled
[    0.360985] CPU features: 0x00002,00002000
[    0.361042] Memory Limit: none

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  4:25         ` Jason A. Donenfeld
@ 2020-03-19  4:36           ` Jason A. Donenfeld
  0 siblings, 0 replies; 11+ messages in thread
From: Jason A. Donenfeld @ 2020-03-19  4:36 UTC (permalink / raw)
  To: Eric Biggers
  Cc: LKML, Linux Crypto Mailing List, Greg Kroah-Hartman, Herbert Xu,
	Emil Renner Berthing, Ard Biesheuvel, stable

On Wed, Mar 18, 2020 at 10:25 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Wed, Mar 18, 2020 at 9:25 PM Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > On Wed, Mar 18, 2020 at 08:27:32PM -0600, Jason A. Donenfeld wrote:
> > > It also fixes up a bug in the (optional, costly) stride test that
> > > prevented it from running on arm64.
> > [...]
> > > diff --git a/lib/crypto/chacha20poly1305-selftest.c b/lib/crypto/chacha20poly1305-selftest.c
> > > index c391a91364e9..fa43deda2660 100644
> > > --- a/lib/crypto/chacha20poly1305-selftest.c
> > > +++ b/lib/crypto/chacha20poly1305-selftest.c
> > > @@ -9028,10 +9028,15 @@ bool __init chacha20poly1305_selftest(void)
> > >            && total_len <= 1 << 10; ++total_len) {
> > >               for (i = 0; i <= total_len; ++i) {
> > >                       for (j = i; j <= total_len; ++j) {
> > > +                             k = 0;
> > >                               sg_init_table(sg_src, 3);
> > > -                             sg_set_buf(&sg_src[0], input, i);
> > > -                             sg_set_buf(&sg_src[1], input + i, j - i);
> > > -                             sg_set_buf(&sg_src[2], input + j, total_len - j);
> > > +                             if (i)
> > > +                                     sg_set_buf(&sg_src[k++], input, i);
> > > +                             if (j - i)
> > > +                                     sg_set_buf(&sg_src[k++], input + i, j - i);
> > > +                             if (total_len - j)
> > > +                                     sg_set_buf(&sg_src[k++], input + j, total_len - j);
> > > +                             sg_init_marker(sg_src, k);
> > >                               memset(computed_output, 0, total_len);
> > >                               memset(input, 0, total_len);
> >
> > So with this test fix, does this test find the bug?
>
> Yes.
>
> > Apparently the empty scatterlist elements caused some problem?  What was
> > that problem exactly?  And what do you mean by this "prevented the test
> > from running on arm64"?  If there is a problem it seems we should
> > something else about about it, e.g. debug checks that work in consistent
> > way on all architectures, documenting what the function expects, or make
> > it just work properly with empty scatterlist elements.
>
> Yea, my next plan was to look into what's going on there. In case
> you're curious, here's what happens:
>
> [    0.355761] [ffffffff041e9508] pgd=000000004ffe9003,
> pud=000000004ffe9003, pmd=0000000000000000
> [    0.356212] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [    0.356587] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc5+ #10
> [    0.356695] Hardware name: linux,dummy-virt (DT)
> [    0.356972] pstate: 20000005 (nzCv daif -PAN -UAO)
> [    0.357158] pc : scatterwalk_copychunks+0x13c/0x1e0
> [    0.357257] lr : scatterwalk_copychunks+0xe0/0x1e0
> [    0.357379] sp : ffffff800f82f970
> [    0.357450] x29: ffffff800f82f970 x28: ffffff800f838000
> [    0.357607] x27: ffffff800f838000 x26: 0000000000000001
> [    0.357725] x25: ffffff800f82faf0 x24: ffffff800f82fa10
> [    0.357820] x23: 0000000000000010 x22: 0000000000000000
> [    0.357898] x21: 0000000000000000 x20: 0000000100200000
> [    0.357978] x19: ffffff8000000000 x18: 0000000000000014
> [    0.358132] x17: 0000000000000000 x16: 0000000000000002
> [    0.358214] x15: 00000000074c5755 x14: 0000000000000002
> [    0.358310] x13: 074c57550311ac67 x12: 09579e470077af1a
> [    0.358453] x11: 1d3f98018ec6f5bb x10: 3e6144a223343e11
> [    0.358560] x9 : 0000000000000000 x8 : ffffff800f82fcc0
> [    0.358638] x7 : 0000000000000000 x6 : ffffff800fa55000
> [    0.358711] x5 : 0000000000001000 x4 : 0000000000000000
> [    0.358781] x3 : ffffffff001e9540 x2 : ffffffff001e9540
> [    0.358855] x1 : ffffffff041e9500 x0 : ffffff800f82fd60
> [    0.359007] Call trace:
> [    0.359177]  scatterwalk_copychunks+0x13c/0x1e0
> [    0.359269]  scatterwalk_map_and_copy+0x50/0xa8
> [    0.359339]  chacha20poly1305_crypt_sg_inplace+0x3f4/0x418
> [    0.359418]  chacha20poly1305_encrypt_sg_inplace+0x10/0x18
> [    0.359511]  chacha20poly1305_selftest+0x4dc/0x618
> [    0.359581]  mod_init+0xc/0x2c
> [    0.359630]  do_one_initcall+0x58/0x11c
> [    0.359687]  kernel_init_freeable+0x174/0x1e0
> [    0.359751]  kernel_init+0x10/0x100
> [    0.359803]  ret_from_fork+0x10/0x18
> [    0.360074] Code: f9400002 530c7c21 927ef442 8b011841 (f9400423)
> [    0.360257] ---[ end trace 0b64264f8a25dbdf ]---
> [    0.360453] Kernel panic - not syncing: Fatal exception
> [    0.360597] SMP: stopping secondary CPUs
> [    0.360835] Kernel Offset: disabled
> [    0.360985] CPU features: 0x00002,00002000
> [    0.361042] Memory Limit: none

__read_once_size at include/linux/compiler.h:199
(inlined by) compound_head at include/linux/page-flags.h:174
(inlined by) PageSlab at include/linux/page-flags.h:325
(inlined by) scatterwalk_pagedone at include/crypto/scatterwalk.h:88
(inlined by) scatterwalk_copychunks at crypto/scatterwalk.c:50

Bug makes sense.

scatterwalk_copychunks->scatterwalk_pagedone->PageSlab, but length
zero page means we're looking at nonsense.

So this v2 seems to be the correct fix.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  2:27     ` [PATCH URGENT crypto v2] " Jason A. Donenfeld
  2020-03-19  3:25       ` Eric Biggers
@ 2020-03-19 19:03       ` Eric Biggers
  2020-03-20  3:48       ` Herbert Xu
  2 siblings, 0 replies; 11+ messages in thread
From: Eric Biggers @ 2020-03-19 19:03 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, gregkh, herbert,
	Emil Renner Berthing, Ard Biesheuvel, stable

On Wed, Mar 18, 2020 at 08:27:32PM -0600, Jason A. Donenfeld wrote:
> Prior, passing in chunks of 2, 3, or 4, followed by any additional
> chunks would result in the chacha state counter getting out of sync,
> resulting in incorrect encryption/decryption, which is a pretty nasty
> crypto vuln: "why do images look weird on webpages?" WireGuard users
> never experienced this prior, because we have always, out of tree, used
> a different crypto library, until the recent Frankenzinc addition. This
> commit fixes the issue by advancing the pointers and state counter by
> the actual size processed. It also fixes up a bug in the (optional,
> costly) stride test that prevented it from running on arm64.
> 
> Fixes: b3aad5bad26a ("crypto: arm64/chacha - expose arm64 ChaCha routine as library function")
> Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: stable@vger.kernel.org # v5.5+
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  arch/arm64/crypto/chacha-neon-glue.c   |  8 ++++----
>  lib/crypto/chacha20poly1305-selftest.c | 11 ++++++++---
>  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/crypto/chacha-neon-glue.c b/arch/arm64/crypto/chacha-neon-glue.c
> index c1f9660d104c..37ca3e889848 100644
> --- a/arch/arm64/crypto/chacha-neon-glue.c
> +++ b/arch/arm64/crypto/chacha-neon-glue.c
> @@ -55,10 +55,10 @@ static void chacha_doneon(u32 *state, u8 *dst, const u8 *src,
>  			break;
>  		}
>  		chacha_4block_xor_neon(state, dst, src, nrounds, l);
> -		bytes -= CHACHA_BLOCK_SIZE * 5;
> -		src += CHACHA_BLOCK_SIZE * 5;
> -		dst += CHACHA_BLOCK_SIZE * 5;
> -		state[12] += 5;
> +		bytes -= l;
> +		src += l;
> +		dst += l;
> +		state[12] += DIV_ROUND_UP(l, CHACHA_BLOCK_SIZE);
>  	}
>  }
>  
> diff --git a/lib/crypto/chacha20poly1305-selftest.c b/lib/crypto/chacha20poly1305-selftest.c
> index c391a91364e9..fa43deda2660 100644
> --- a/lib/crypto/chacha20poly1305-selftest.c
> +++ b/lib/crypto/chacha20poly1305-selftest.c
> @@ -9028,10 +9028,15 @@ bool __init chacha20poly1305_selftest(void)
>  	     && total_len <= 1 << 10; ++total_len) {
>  		for (i = 0; i <= total_len; ++i) {
>  			for (j = i; j <= total_len; ++j) {
> +				k = 0;
>  				sg_init_table(sg_src, 3);
> -				sg_set_buf(&sg_src[0], input, i);
> -				sg_set_buf(&sg_src[1], input + i, j - i);
> -				sg_set_buf(&sg_src[2], input + j, total_len - j);
> +				if (i)
> +					sg_set_buf(&sg_src[k++], input, i);
> +				if (j - i)
> +					sg_set_buf(&sg_src[k++], input + i, j - i);
> +				if (total_len - j)
> +					sg_set_buf(&sg_src[k++], input + j, total_len - j);
> +				sg_init_marker(sg_src, k);
>  				memset(computed_output, 0, total_len);
>  				memset(input, 0, total_len);
>  

Reviewed-by: Eric Biggers <ebiggers@google.com>

Herbert, can you send this to Linus for 5.6?

- Eric

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-19  2:27     ` [PATCH URGENT crypto v2] " Jason A. Donenfeld
  2020-03-19  3:25       ` Eric Biggers
  2020-03-19 19:03       ` Eric Biggers
@ 2020-03-20  3:48       ` Herbert Xu
  2020-03-20  4:01         ` Jason A. Donenfeld
  2 siblings, 1 reply; 11+ messages in thread
From: Herbert Xu @ 2020-03-20  3:48 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, gregkh, Emil Renner Berthing,
	Ard Biesheuvel, stable

On Wed, Mar 18, 2020 at 08:27:32PM -0600, Jason A. Donenfeld wrote:
> Prior, passing in chunks of 2, 3, or 4, followed by any additional
> chunks would result in the chacha state counter getting out of sync,
> resulting in incorrect encryption/decryption, which is a pretty nasty
> crypto vuln: "why do images look weird on webpages?" WireGuard users
> never experienced this prior, because we have always, out of tree, used
> a different crypto library, until the recent Frankenzinc addition. This
> commit fixes the issue by advancing the pointers and state counter by
> the actual size processed. It also fixes up a bug in the (optional,
> costly) stride test that prevented it from running on arm64.
> 
> Fixes: b3aad5bad26a ("crypto: arm64/chacha - expose arm64 ChaCha routine as library function")
> Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: stable@vger.kernel.org # v5.5+
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  arch/arm64/crypto/chacha-neon-glue.c   |  8 ++++----
>  lib/crypto/chacha20poly1305-selftest.c | 11 ++++++++---
>  2 files changed, 12 insertions(+), 7 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks
  2020-03-20  3:48       ` Herbert Xu
@ 2020-03-20  4:01         ` Jason A. Donenfeld
  0 siblings, 0 replies; 11+ messages in thread
From: Jason A. Donenfeld @ 2020-03-20  4:01 UTC (permalink / raw)
  To: Herbert Xu
  Cc: LKML, Linux Crypto Mailing List, Greg Kroah-Hartman,
	Emil Renner Berthing, Ard Biesheuvel, stable

On Thu, Mar 19, 2020 at 9:48 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Wed, Mar 18, 2020 at 08:27:32PM -0600, Jason A. Donenfeld wrote:
> > Prior, passing in chunks of 2, 3, or 4, followed by any additional
> > chunks would result in the chacha state counter getting out of sync,
> > resulting in incorrect encryption/decryption, which is a pretty nasty
> > crypto vuln: "why do images look weird on webpages?" WireGuard users
> > never experienced this prior, because we have always, out of tree, used
> > a different crypto library, until the recent Frankenzinc addition. This
> > commit fixes the issue by advancing the pointers and state counter by
> > the actual size processed. It also fixes up a bug in the (optional,
> > costly) stride test that prevented it from running on arm64.
> >
> > Fixes: b3aad5bad26a ("crypto: arm64/chacha - expose arm64 ChaCha routine as library function")
> > Reported-and-tested-by: Emil Renner Berthing <kernel@esmil.dk>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: stable@vger.kernel.org # v5.5+
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> >  arch/arm64/crypto/chacha-neon-glue.c   |  8 ++++----
> >  lib/crypto/chacha20poly1305-selftest.c | 11 ++++++++---
> >  2 files changed, 12 insertions(+), 7 deletions(-)
>
> Patch applied.  Thanks.

Thanks! No idea whether Linus will skip a 5.6-rc7 with people not at
work due to the quarantines, so given the gravity of this bug, it
might be prudent to send a PR to him _now_, rather then waiting until
next week.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-03-20  4:02 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 23:45 [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks Jason A. Donenfeld
2020-03-19  0:23 ` Eric Biggers
2020-03-19  0:30   ` Eric Biggers
2020-03-19  1:33   ` Jason A. Donenfeld
2020-03-19  2:27     ` [PATCH URGENT crypto v2] " Jason A. Donenfeld
2020-03-19  3:25       ` Eric Biggers
2020-03-19  4:25         ` Jason A. Donenfeld
2020-03-19  4:36           ` Jason A. Donenfeld
2020-03-19 19:03       ` Eric Biggers
2020-03-20  3:48       ` Herbert Xu
2020-03-20  4:01         ` Jason A. Donenfeld

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).