[PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks

[PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks

[Jason A. Donenfeld]

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

Re: [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks

[Eric Biggers]

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

Re: [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks

[Eric Biggers]

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+

Re: [PATCH URGENT crypto] crypto: arm64/chacha - correctly walk through blocks

[Jason A. Donenfeld]

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

[PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Jason A. Donenfeld]

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

Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Eric Biggers]

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

Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Jason A. Donenfeld]

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

Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Jason A. Donenfeld]

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.

Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Eric Biggers]

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

Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Herbert Xu]

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

Re: [PATCH URGENT crypto v2] crypto: arm64/chacha - correctly walk through blocks

[Jason A. Donenfeld]

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