crypto: skcipher - fix crash in virtual walk

Message ID	1481636042-27347-1-git-send-email-ard.biesheuvel@linaro.org
State	Accepted
Commit	18e615ad87bce9125ef3990377a4a946ec0f21f3
Headers	show Delivered-To: patch@linaro.org Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; From: Ard Biesheuvel <ard.biesheuvel@linaro.org> To: linux-crypto@vger.kernel.org Cc: herbert@gondor.apana.org.au, Ard Biesheuvel <ard.biesheuvel@linaro.org> Subject: [PATCH] crypto: skcipher - fix crash in virtual walk Date: Tue, 13 Dec 2016 13:34:02 +0000 Message-Id: <1481636042-27347-1-git-send-email-ard.biesheuvel@linaro.org> Sender: linux-crypto-owner@vger.kernel.org Precedence: bulk

Message ID

1481636042-27347-1-git-send-email-ard.biesheuvel@linaro.org

State

Accepted

Commit

18e615ad87bce9125ef3990377a4a946ec0f21f3

Headers

Received-SPF: pass (google.com: best guess record for domain of
	linux-crypto-owner@vger.kernel.org designates 209.132.180.67
	as permitted sender) client-ip=209.132.180.67; 
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: linux-crypto@vger.kernel.org
Cc: herbert@gondor.apana.org.au, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH] crypto: skcipher - fix crash in virtual walk
Date: Tue, 13 Dec 2016 13:34:02 +0000
Message-Id: <1481636042-27347-1-git-send-email-ard.biesheuvel@linaro.org>
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

Commit Message

Ard Biesheuvel Dec. 13, 2016, 1:34 p.m. UTC

The new skcipher walk API may crash in the following way. (Interestingly,
the tcrypt boot time tests seem unaffected, while an explicit test using
the module triggers it)

  Unable to handle kernel NULL pointer dereference at virtual address 00000000
  ...
  [<ffff000008431d84>] __memcpy+0x84/0x180
  [<ffff0000083ec0d0>] skcipher_walk_done+0x328/0x340
  [<ffff0000080c5c04>] ctr_encrypt+0x84/0x100
  [<ffff000008406d60>] simd_skcipher_encrypt+0x88/0x98
  [<ffff0000083fa05c>] crypto_rfc3686_crypt+0x8c/0x98
  [<ffff0000009b0900>] test_skcipher_speed+0x518/0x820 [tcrypt]
  [<ffff0000009b31c0>] do_test+0x1408/0x3b70 [tcrypt]
  [<ffff0000009bd050>] tcrypt_mod_init+0x50/0x1000 [tcrypt]
  [<ffff0000080838f4>] do_one_initcall+0x44/0x138
  [<ffff0000081aee60>] do_init_module+0x68/0x1e0
  [<ffff0000081524d0>] load_module+0x1fd0/0x2458
  [<ffff000008152c38>] SyS_finit_module+0xe0/0xf0
  [<ffff0000080836f0>] el0_svc_naked+0x24/0x28

This is due to the fact that skcipher_done_slow() may be entered with
walk->buffer unset. Since skcipher_walk_done() already deals with the
case where walk->buffer == walk->page, it appears to be the intention
that walk->buffer point to walk->page after skcipher_next_slow(), so
ensure that is the case.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 crypto/skcipher.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Herbert Xu Dec. 14, 2016, 10:39 a.m. UTC | #1

On Tue, Dec 13, 2016 at 01:34:02PM +0000, Ard Biesheuvel wrote:
> The new skcipher walk API may crash in the following way. (Interestingly,

> the tcrypt boot time tests seem unaffected, while an explicit test using

> the module triggers it)

> 

>   Unable to handle kernel NULL pointer dereference at virtual address 00000000

>   ...

>   [<ffff000008431d84>] __memcpy+0x84/0x180

>   [<ffff0000083ec0d0>] skcipher_walk_done+0x328/0x340

>   [<ffff0000080c5c04>] ctr_encrypt+0x84/0x100

>   [<ffff000008406d60>] simd_skcipher_encrypt+0x88/0x98

>   [<ffff0000083fa05c>] crypto_rfc3686_crypt+0x8c/0x98

>   [<ffff0000009b0900>] test_skcipher_speed+0x518/0x820 [tcrypt]

>   [<ffff0000009b31c0>] do_test+0x1408/0x3b70 [tcrypt]

>   [<ffff0000009bd050>] tcrypt_mod_init+0x50/0x1000 [tcrypt]

>   [<ffff0000080838f4>] do_one_initcall+0x44/0x138

>   [<ffff0000081aee60>] do_init_module+0x68/0x1e0

>   [<ffff0000081524d0>] load_module+0x1fd0/0x2458

>   [<ffff000008152c38>] SyS_finit_module+0xe0/0xf0

>   [<ffff0000080836f0>] el0_svc_naked+0x24/0x28

> 

> This is due to the fact that skcipher_done_slow() may be entered with

> walk->buffer unset. Since skcipher_walk_done() already deals with the

> case where walk->buffer == walk->page, it appears to be the intention

> that walk->buffer point to walk->page after skcipher_next_slow(), so

> ensure that is the case.

> 

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Milan Broz Dec. 15, 2016, 1:27 p.m. UTC | #2

On 12/14/2016 11:39 AM, Herbert Xu wrote:
> On Tue, Dec 13, 2016 at 01:34:02PM +0000, Ard Biesheuvel wrote:

>> The new skcipher walk API may crash in the following way. (Interestingly,

>> the tcrypt boot time tests seem unaffected, while an explicit test using

>> the module triggers it)

>>

>>   Unable to handle kernel NULL pointer dereference at virtual address 00000000

>>   ...

>>   [<ffff000008431d84>] __memcpy+0x84/0x180

>>   [<ffff0000083ec0d0>] skcipher_walk_done+0x328/0x340

>>   [<ffff0000080c5c04>] ctr_encrypt+0x84/0x100

>>   [<ffff000008406d60>] simd_skcipher_encrypt+0x88/0x98

>>   [<ffff0000083fa05c>] crypto_rfc3686_crypt+0x8c/0x98

>>   [<ffff0000009b0900>] test_skcipher_speed+0x518/0x820 [tcrypt]

>>   [<ffff0000009b31c0>] do_test+0x1408/0x3b70 [tcrypt]

>>   [<ffff0000009bd050>] tcrypt_mod_init+0x50/0x1000 [tcrypt]

>>   [<ffff0000080838f4>] do_one_initcall+0x44/0x138

>>   [<ffff0000081aee60>] do_init_module+0x68/0x1e0

>>   [<ffff0000081524d0>] load_module+0x1fd0/0x2458

>>   [<ffff000008152c38>] SyS_finit_module+0xe0/0xf0

>>   [<ffff0000080836f0>] el0_svc_naked+0x24/0x28

>>

>> This is due to the fact that skcipher_done_slow() may be entered with

>> walk->buffer unset. Since skcipher_walk_done() already deals with the

>> case where walk->buffer == walk->page, it appears to be the intention

>> that walk->buffer point to walk->page after skcipher_next_slow(), so

>> ensure that is the case.

>>

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> 

> Patch applied.  Thanks.


Fixed problem here as well...
Tested-by: Milan Broz <gmazyland@gmail.com>


Without patch, just running cryptsetup tests (make check) on 32bit machine
I get kernel crash in LRW mode.

For me, the problem can be triggered from the userspace crypto API (no root needed!)
  dd if=/dev/zero of=test bs=1M count=32
  echo blah | /sbin/cryptsetup luksFormat test -c aes-lrw-null -q --use-urandom

and I get

Dec 15 13:00:50 kernel: NET: Registered protocol family 38
Dec 15 13:00:50 kernel: BUG: unable to handle kernel NULL pointer dereference at   (null)
Dec 15 13:00:50 kernel: IP: memcpy+0xf/0x20
Dec 15 13:00:50 kernel: *pde = 00000000 
Dec 15 13:00:50 kernel: 
Dec 15 13:00:50 kernel: Oops: 0000 [#1] PREEMPT SMP
Dec 15 13:00:50 kernel: Modules linked in: lrw algif_skcipher af_alg loop rpcsec_gss_krb5 dm_mod crc32_pclmul crc32c_intel pcbc aesni_intel aes_i586 crypto_simd cryptd ata_piix
Dec 15 13:00:50 kernel: CPU: 0 PID: 1667 Comm: cryptsetup Tainted: G        W       4.9.0+ #122
Dec 15 13:00:50 kernel: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/02/2015
Dec 15 13:00:50 kernel: task: f5cc66c0 task.stack: f45e6000
Dec 15 13:00:50 kernel: EIP: memcpy+0xf/0x20
Dec 15 13:00:50 kernel: EFLAGS: 00010202 CPU: 0
Dec 15 13:00:50 kernel: EAX: fffbaffc EBX: 00000004 ECX: 00000001 EDX: 00000000
Dec 15 13:00:50 kernel: ESI: 00000000 EDI: fffbaffc EBP: f45e7d48 ESP: f45e7d3c
Dec 15 13:00:50 kernel:  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Dec 15 13:00:50 kernel: CR0: 80050033 CR2: 00000000 CR3: 35eba000 CR4: 001406d0
Dec 15 13:00:50 kernel: Call Trace:
Dec 15 13:00:50 kernel:  scatterwalk_copychunks+0x75/0xd0
Dec 15 13:00:50 kernel:  skcipher_walk_done+0x2b1/0x300
Dec 15 13:00:50 kernel:  pre_crypt+0x230/0x350 [lrw]
Dec 15 13:00:50 kernel:  ? __kmalloc+0x222/0x270
Dec 15 13:00:50 kernel:  do_decrypt+0x27/0xb0 [lrw]
Dec 15 13:00:50 kernel:  decrypt+0x19/0x20 [lrw]
Dec 15 13:00:50 kernel:  skcipher_recvmsg+0x2d3/0x6c0 [algif_skcipher]
Dec 15 13:00:50 kernel:  ? trace_hardirqs_on_caller+0xd6/0x200
Dec 15 13:00:50 kernel:  ? release_sock+0x63/0x90
Dec 15 13:00:50 kernel:  ? trace_hardirqs_on+0xb/0x10
Dec 15 13:00:50 kernel:  ? __local_bh_enable_ip+0x5c/0xd0
Dec 15 13:00:50 kernel:  ? _raw_spin_unlock_bh+0x2a/0x30
Dec 15 13:00:50 kernel:  skcipher_recvmsg_nokey+0x26/0x38 [algif_skcipher]
Dec 15 13:00:50 kernel:  sock_read_iter+0x77/0xb0
Dec 15 13:00:50 kernel:  __vfs_read+0xaa/0x120
Dec 15 13:00:50 kernel:  vfs_read+0x72/0x100
Dec 15 13:00:50 kernel:  SyS_read+0x3d/0xa0
Dec 15 13:00:50 kernel:  do_int80_syscall_32+0x40/0x110
Dec 15 13:00:50 kernel:  entry_INT80_32+0x2f/0x2f
Dec 15 13:00:50 kernel: EIP: 0xb77cc9f2
Dec 15 13:00:50 kernel: EFLAGS: 00000246 CPU: 0
Dec 15 13:00:50 kernel: EAX: ffffffda EBX: 00000006 ECX: bff01f4c EDX: 00000200
Dec 15 13:00:50 kernel: ESI: 00000030 EDI: fffffffb EBP: bff01e78 ESP: bff01dd4
Dec 15 13:00:50 kernel:  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b
Dec 15 13:00:50 kernel: Code: fc ff ff 8b 43 58 2b 43 50 88 43 4e 5b 5d c3 90 90 90 90 90 90 90 90 90 90 90 90 90 55 89 e5 57 89 c7 56 89 d6 53 89 cb c1 e9 02 <f3> a5 89 d9 83 e1 03 74 02 f3 a4 5b 5e 5f 5d c3 90 55 89 e5 57
Dec 15 13:00:50 kernel: EIP: memcpy+0xf/0x20 SS:ESP: 0068:f45e7d3c
Dec 15 13:00:50 kernel: CR2: 0000000000000000
Dec 15 13:00:50 kernel: ---[ end trace 6d5fbbd95de42602 ]---
Dec 15 13:00:50 kernel: note: cryptsetup[1667] exited with preempt_count 1


--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index aca07c643d41..0e1e6c35188e 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -226,7 +226,9 @@  static int skcipher_next_slow(struct skcipher_walk *walk, unsigned int bsize)
 	void *v;
 
 	if (!phys) {
-		buffer = walk->buffer ?: walk->page;
+		if (!walk->buffer)
+			walk->buffer = walk->page;
+		buffer = walk->buffer;
 		if (buffer)
 			goto ok;
 	}