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; } }
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
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+
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
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);
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
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
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.
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
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.
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
linux-stable-mirror@lists.linaro.org
-
Eric Biggers -
Herbert Xu -
Jason A. Donenfeld