[PATCH 0/2] fix authenc() kernel crash

[PATCH 0/2] fix authenc() kernel crash

[Stephan Müller]

Hi Herbert,

The two patches together fix a kernel crash that can be triggered via
AF_ALG when using authenc() with zero plaintext.

The changes are also tested to verify that the hashing on null data
is still supported.

I suspect that the vulnerability fixed with patch 1 is present in
abklcipher that was used before the switch to skcipher. Thus, I would
suspect in older kernels that this vulnerability is still present.
Could you please provide guidance on how to address that issue in such
older kernels?

Stephan Mueller (2):
  crypto: skcipher - noop for enc/dec with NULL data
  crypto: shash - no kmap of zero SG

 crypto/shash.c            | 4 +++-
 include/crypto/skcipher.h | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.13.5

[PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

The encryption / decryption operation is a noop in case the caller
provides zero input data. As this noop is a "valid" operation, the API
calls will return no error, but simply skip any processing.

This fixes a kernel crash with authenc() ciphers and zero plaintext /
ciphertext that can be triggered via AF_ALG from unprivileged user
space.

Fixes: 7a7ffe65c8c5f ("crypto: skcipher - Add top-level skcipher
interface")
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: <stable@vger.kernel.org>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 include/crypto/skcipher.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 562001cb412b..ca27fbadbe67 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -442,6 +442,9 @@ static inline int crypto_skcipher_encrypt(struct skcipher_request *req)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 
+	if (!req->cryptlen)
+		return 0;
+
 	return tfm->encrypt(req);
 }
 
@@ -460,6 +463,9 @@ static inline int crypto_skcipher_decrypt(struct skcipher_request *req)
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 
+	if (!req->cryptlen)
+		return 0;
+
 	return tfm->decrypt(req);
 }
 
-- 
2.13.5

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Herbert Xu]

On Sun, Sep 24, 2017 at 08:24:58AM +0200, Stephan Müller wrote:
> The encryption / decryption operation is a noop in case the caller
> provides zero input data. As this noop is a "valid" operation, the API
> calls will return no error, but simply skip any processing.
> 
> This fixes a kernel crash with authenc() ciphers and zero plaintext /
> ciphertext that can be triggered via AF_ALG from unprivileged user
> space.
> 
> Fixes: 7a7ffe65c8c5f ("crypto: skcipher - Add top-level skcipher
> interface")
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>

Hmm this just papers over bugs in the underlying code.  Which
algorithm is causing the crash with a zero input? They're supposed
to handle this case.

Cheers,
-- 
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 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 04:46:35 CEST schrieb Herbert Xu:

Hi Herbert,

> Hmm this just papers over bugs in the underlying code.  Which
> algorithm is causing the crash with a zero input? They're supposed
> to handle this case.

The bug happens with authenc. It is surely possible to modify authenc. Yet I 
thought that covering such issue at a central spot at least prevents similar 
buts to be exploitable from userspace.

Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Herbert Xu]

On Sat, Oct 07, 2017 at 04:49:10AM +0200, Stephan Müller wrote:
> Am Samstag, 7. Oktober 2017, 04:46:35 CEST schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > Hmm this just papers over bugs in the underlying code.  Which
> > algorithm is causing the crash with a zero input? They're supposed
> > to handle this case.
> 
> The bug happens with authenc. It is surely possible to modify authenc. Yet I 
> thought that covering such issue at a central spot at least prevents similar 
> buts to be exploitable from userspace.

No I'm talking about the underlysing skcipher.  authenc is an
aead, and it is legal to make a zero skcipher call.  The underlying
skcipher should make sure that it works.

So which underlying skcipher is barfing over a zero input?

Cheers,
-- 
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 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 04:51:17 CEST schrieb Herbert Xu:

Hi Herbert,

> 
> No I'm talking about the underlysing skcipher.  authenc is an
> aead, and it is legal to make a zero skcipher call.  The underlying
> skcipher should make sure that it works.
> 
> So which underlying skcipher is barfing over a zero input?

I use authenc(hmac(sha256),cbc(aes)) which in turn uses cbc-aes-aesni on my 
system.

Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Herbert Xu]

On Sat, Oct 07, 2017 at 04:53:46AM +0200, Stephan Müller wrote:
>
> I use authenc(hmac(sha256),cbc(aes)) which in turn uses cbc-aes-aesni on my 
> system.

So where exactly is it crashing in cbc-aes-aesni? AFAICS it should
handle the zero case just fine.  Or is it hmac that's crashing as
your other patch suggested?

Cheers,
-- 
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 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 05:07:52 CEST schrieb Herbert Xu:

Hi Herbert,

> On Sat, Oct 07, 2017 at 04:53:46AM +0200, Stephan Müller wrote:
> > I use authenc(hmac(sha256),cbc(aes)) which in turn uses cbc-aes-aesni on
> > my
> > system.
> 
> So where exactly is it crashing in cbc-aes-aesni? AFAICS it should
> handle the zero case just fine.  Or is it hmac that's crashing as
> your other patch suggested?

The bug happens with the following invocation sequence:

setsockopt(3, SOL_ALG, 5, NULL, 1)      = -1 EBUSY (Device or resource busy)
sendmsg(6, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0, 
msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}, {cmsg_len=40, 
cmsg_level=SOL_ALG, cmsg_type=0x2}, {cmsg_len=20, cmsg_level=SOL_ALG, 
cmsg_type=0x4}], msg_controllen=88, msg_flags=0}, MSG_MORE) = 0
vmsplice(5, [{iov_base="V", iov_len=1}], 1, SPLICE_F_GIFT) = 1
splice(4, NULL, 6, NULL, 1, 0)          = 1
read(6,  <unfinished ...>)              = ?
+++ killed by SIGKILL +++

The kernel reports the following crash:

[135385.003653] BUG: unable to handle kernel NULL pointer dereference at 
0000000000000010
[135385.004007] IP: skcipher_walk_skcipher+0x18/0xb0
[135385.004007] PGD 7bbf4067 P4D 7bbf4067 PUD 784a6067 PMD 0 
[135385.004007] Oops: 0000 [#1] SMP
[135385.004007] Modules linked in: authenc algif_aead algif_rng algif_skcipher 
crypto_user algif_hash af_alg ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 
nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack 
nf_conntrack ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc 
ip6table_mangle ip6table_raw ip6table_security iptable_mangle iptable_raw 
iptable_security ebtable_filter ebtables ip6table_filter ip6_tables 
crct10dif_pclmul crc32_pclmul ghash_clmulni_intel virtio_net virtio_balloon 
pcspkr i2c_piix4 sch_fq_codel virtio_blk virtio_console virtio_pci 
crc32c_intel virtio_ring serio_raw virtio
[135385.004007] CPU: 3 PID: 1148 Comm: lt-kcapi Not tainted 4.14.0-rc1+ #554
[135385.004007] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[135385.004007] task: ffff976fb9380d40 task.stack: ffff9fd280e24000
[135385.004007] RIP: 0010:skcipher_walk_skcipher+0x18/0xb0
[135385.004007] RSP: 0018:ffff9fd280e27ba0 EFLAGS: 00010246
[135385.004007] RAX: 0000000000000000 RBX: ffff9fd280e27be0 RCX: 
0000000000000000
[135385.004007] RDX: ffff976fb85a5428 RSI: ffff976f76330d08 RDI: 
ffff9fd280e27be0
[135385.004007] RBP: ffff9fd280e27bc0 R08: 0000000087654321 R09: 
ffff976fb842b880
[135385.004007] R10: ffff9fd280e27cb8 R11: 0000000000000000 R12: 
0000000000000001
[135385.004007] R13: ffff976f76330d08 R14: ffff976fb842b800 R15: 
0000000000000000
[135385.004007] FS:  00007fcb922bb700(0000) GS:ffff976fbfd80000(0000) knlGS:
0000000000000000
[135385.004007] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[135385.004007] CR2: 0000000000000010 CR3: 00000000798c4001 CR4: 
00000000003606e0
[135385.004007] Call Trace:
[135385.004007]  ? skcipher_walk_virt+0x1e/0x40
[135385.004007]  cbc_encrypt+0x3e/0xc0
[135385.004007]  ? skcipher_null_crypt+0x64/0x80
[135385.004007]  simd_skcipher_encrypt+0xb7/0xc0
[135385.004007]  ? simd_skcipher_encrypt+0xb7/0xc0
[135385.004007]  crypto_authenc_encrypt+0x94/0x170 [authenc]
[135385.004007]  aead_recvmsg+0x2dd/0x5f0 [algif_aead]
[135385.004007]  sock_recvmsg+0x3d/0x50
[135385.004007]  sock_read_iter+0x86/0xc0
[135385.004007]  __vfs_read+0xcb/0x120
[135385.004007]  vfs_read+0x8e/0x130
[135385.004007]  SyS_read+0x46/0xa0
[135385.004007]  do_syscall_64+0x5f/0xf0
[135385.004007]  entry_SYSCALL64_slow_path+0x25/0x25
[135385.004007] RIP: 0033:0x7fcb91bc71b0
[135385.004007] RSP: 002b:00007ffe41fc2898 EFLAGS: 00000246 ORIG_RAX: 
0000000000000000
[135385.004007] RAX: ffffffffffffffda RBX: 0000000000001000 RCX: 
00007fcb91bc71b0
[135385.004007] RDX: 0000000000001000 RSI: 00007ffe41fc39a0 RDI: 
0000000000000006
[135385.004007] RBP: 00000000012f601c R08: 0000000000000001 R09: 
0000000000000000
[135385.004007] R10: 0000000000000000 R11: 0000000000000246 R12: 
0000000000000000
[135385.004007] R13: 00007ffe41fc39a0 R14: 00007ffe41fc39a0 R15: 
00007ffe41fc29a0
[135385.004007] Code: ff ff ff e9 42 ff ff ff 90 66 2e 0f 1f 84 00 00 00 00 00 
0f 1f 44 00 00 48 8b 46 10 48 8b 56 40 55 8b 8f 84 00 00 00 48 89 47 20 <8b> 
40 10 48 89 e5 83 e1 ef 89 47 28 48 8b 46 18 48 89 47 38 8b 
[135385.004007] RIP: skcipher_walk_skcipher+0x18/0xb0 RSP: ffff9fd280e27ba0
[135385.004007] CR2: 0000000000000010
[135385.004007] ---[ end trace 25c44edb63da431d ]---
[135385.004007] Kernel panic - not syncing: Fatal exception
[135385.004007] Kernel Offset: 0x11000000 from 0xffffffff81000000 (relocation 
range: 0xffffffff80000000-0xffffffffbfffffff)
[135385.004007] ---[ end Kernel panic - not syncing: Fatal exception
> 
> Cheers,


Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Herbert Xu]

On Sat, Oct 07, 2017 at 05:21:38AM +0200, Stephan Müller wrote:
>
> The bug happens with the following invocation sequence:
> 
> setsockopt(3, SOL_ALG, 5, NULL, 1)      = -1 EBUSY (Device or resource busy)
> sendmsg(6, {msg_name=NULL, msg_namelen=0, msg_iov=NULL, msg_iovlen=0, 
> msg_control=[{cmsg_len=20, cmsg_level=SOL_ALG, cmsg_type=0x3}, {cmsg_len=40, 
> cmsg_level=SOL_ALG, cmsg_type=0x2}, {cmsg_len=20, cmsg_level=SOL_ALG, 
> cmsg_type=0x4}], msg_controllen=88, msg_flags=0}, MSG_MORE) = 0
> vmsplice(5, [{iov_base="V", iov_len=1}], 1, SPLICE_F_GIFT) = 1
> splice(4, NULL, 6, NULL, 1, 0)          = 1
> read(6,  <unfinished ...>)              = ?
> +++ killed by SIGKILL +++

I see the problem now.  This was introduced with the skcipher walk
interface.  The blkcipher walk interface didn't have this issue.

I guess we should add a zero test vector once this is fixed.

---8<---
Subject: crypto: skcipher - Fix crash on zero-length input

The skcipher walk interface doesn't handle zero-length input
properly as the old blkcipher walk interface did.  This is due
to the fact that the length check is done too late.

This patch moves the length check forward so that it does the
right thing.

Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk...")
Cc: <stable@vger.kernel.org>
Reported-by: Stephan Müller <smueller@chronox.de>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 4faa0fd..d5692e3 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -426,14 +426,9 @@ static int skcipher_copy_iv(struct skcipher_walk *walk)
 
 static int skcipher_walk_first(struct skcipher_walk *walk)
 {
-	walk->nbytes = 0;
-
 	if (WARN_ON_ONCE(in_irq()))
 		return -EDEADLK;
 
-	if (unlikely(!walk->total))
-		return 0;
-
 	walk->buffer = NULL;
 	if (unlikely(((unsigned long)walk->iv & walk->alignmask))) {
 		int err = skcipher_copy_iv(walk);
@@ -452,10 +447,15 @@ static int skcipher_walk_skcipher(struct skcipher_walk *walk,
 {
 	struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(req);
 
+	walk->total = req->cryptlen;
+	walk->nbytes = 0;
+
+	if (unlikely(!walk->total))
+		return 0;
+
 	scatterwalk_start(&walk->in, req->src);
 	scatterwalk_start(&walk->out, req->dst);
 
-	walk->total = req->cryptlen;
 	walk->iv = req->iv;
 	walk->oiv = req->iv;
 
@@ -509,6 +509,11 @@ static int skcipher_walk_aead_common(struct skcipher_walk *walk,
 	struct crypto_aead *tfm = crypto_aead_reqtfm(req);
 	int err;
 
+	walk->nbytes = 0;
+
+	if (unlikely(!walk->total))
+		return 0;
+
 	walk->flags &= ~SKCIPHER_WALK_PHYS;
 
 	scatterwalk_start(&walk->in, req->src);
-- 
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 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Samstag, 7. Oktober 2017, 05:29:48 CEST schrieb Herbert Xu:

Hi Herbert,

> I see the problem now.  This was introduced with the skcipher walk
> interface.  The blkcipher walk interface didn't have this issue.
> 
> I guess we should add a zero test vector once this is fixed.

Thank you.

This fixes the issue. 

Tested-by: Stephan Müller <smueller@chronox.de>

Though, this opens up the shash issue I tried to fix.

[  213.090857] ------------[ cut here ]------------
[  213.091318] kernel BUG at ./include/linux/scatterlist.h:123!
[  213.091986] invalid opcode: 0000 [#1] SMP
[  213.092307] Modules linked in: algif_skcipher crypto_user authenc 
algif_aead af_alg ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 
nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack 
ip_set nfnetlink ebtable_nat ebtable_broute bridge stp llc ip6table_mangle 
ip6table_raw ip6table_security iptable_mangle iptable_raw iptable_security 
ebtable_filter ebtables ip6table_filter ip6_tables crct10dif_pclmul 
crc32_pclmul ghash_clmulni_intel virtio_balloon virtio_net pcspkr i2c_piix4 
sch_fq_codel virtio_blk virtio_console crc32c_intel virtio_pci virtio_ring 
serio_raw virtio
[  213.092307] CPU: 0 PID: 1014 Comm: kcapi Not tainted 4.14.0-rc1+ #555
[  213.092307] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[  213.092307] task: ffff8b8838e94f80 task.stack: ffff9dbe40598000
[  213.092307] RIP: 0010:shash_ahash_digest+0xc9/0xd0
[  213.092307] RSP: 0018:ffff9dbe4059bc38 EFLAGS: 00010286
[  213.092307] RAX: 0000000087654321 RBX: ffff8b883bf64cb8 RCX: 
ffff8b883bf64810
[  213.092307] RDX: 0000000000000000 RSI: ffff8b883bf64d08 RDI: 
000000003bf64858
[  213.092307] RBP: ffff9dbe4059bc58 R08: ffffffff99402320 R09: 
ffff9dbe4059bd70
[  213.092307] R10: ffff9dbe4059bcb8 R11: ffff8b883bf64810 R12: 
ffff8b883bf64d08
[  213.092307] R13: ffff8b8837d1a248 R14: ffff8b8838555980 R15: 
ffff8b883bf64810
[  213.092307] FS:  00007facf85b8700(0000) GS:ffff8b883fc00000(0000) knlGS:
0000000000000000
[  213.092307] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  213.092307] CR2: 0000000000000000 CR3: 000000007845e002 CR4: 
00000000003606f0
[  213.092307] Call Trace:
[  213.092307]  ? shash_ahash_digest+0xd0/0xd0
[  213.092307]  shash_async_digest+0x24/0x30
[  213.092307]  crypto_ahash_op+0x29/0x70
[  213.092307]  crypto_ahash_digest+0x16/0x20
[  213.092307]  crypto_authenc_genicv+0x7b/0xb0 [authenc]
[  213.092307]  ? simd_skcipher_encrypt+0xb7/0xc0
[  213.092307]  crypto_authenc_encrypt+0xa3/0x170 [authenc]
[  213.092307]  aead_recvmsg+0x2dd/0x5f0 [algif_aead]
[  213.092307]  sock_recvmsg+0x3d/0x50
[  213.092307]  sock_read_iter+0x86/0xc0
[  213.092307]  __vfs_read+0xcb/0x120
[  213.092307]  vfs_read+0x8e/0x130
[  213.092307]  SyS_read+0x46/0xa0
[  213.092307]  do_syscall_64+0x5f/0xf0
[  213.092307]  entry_SYSCALL64_slow_path+0x25/0x25
[  213.092307] RIP: 0033:0x7facf7ec41b0
[  213.092307] RSP: 002b:00007ffff79f22b8 EFLAGS: 00000246 ORIG_RAX: 
0000000000000000
[  213.092307] RAX: ffffffffffffffda RBX: 0000000000001000 RCX: 
00007facf7ec41b0
[  213.092307] RDX: 0000000000001000 RSI: 00007ffff79f33c0 RDI: 
0000000000000006
[  213.092307] RBP: 0000000001ad701c R08: 0000000000000000 R09: 
00007ffff79f2250
[  213.092307] R10: 0000000000000000 R11: 0000000000000246 R12: 
0000000000000000
[  213.092307] R13: 00007ffff79f33c0 R14: 00007ffff79f33c0 R15: 
00007ffff79f23c0
[  213.092307] Code: 03 35 14 0e a4 00 48 01 fe 4c 89 e7 e8 71 fa ff ff 41 89 
c5 41 83 ae c0 08 00 00 01 41 f6 44 24 09 02 74 92 e8 79 6f 42 00 eb 8b <0f> 
0b 0f 0b 0f 1f 00 0f 1f 44 00 00 48 8b 47 20 55 48 8d 77 50 
[  213.092307] RIP: shash_ahash_digest+0xc9/0xd0 RSP: ffff9dbe4059bc38
[  213.125048] ---[ end trace 7b80ad4517eb7967 ]---

Ciao
Stephan

Re: [PATCH 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Herbert Xu]

On Sat, Oct 07, 2017 at 02:56:24PM +0200, Stephan Müller wrote:
>
> Though, this opens up the shash issue I tried to fix.

Does this patch fix the crash?

---8<---
Subject: crypto: shash - Fix zero-length shash ahash digest crash

The shash ahash digest adaptor function may crash if given a
zero-length input together with a null SG list.  This is because
it tries to read the SG list before looking at the length.

This patch fixes it by checking the length first.

Cc: <stable@vger.kernel.org>
Reported-by: Stephan Müller<smueller@chronox.de>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/shash.c b/crypto/shash.c
index 8fcecc6..a8322b0 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -275,11 +275,17 @@ static int shash_async_finup(struct ahash_request *req)
 
 int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
 {
-	struct scatterlist *sg = req->src;
-	unsigned int offset = sg->offset;
 	unsigned int nbytes = req->nbytes;
+	struct scatterlist *sg;
+	unsigned int offset;
 	int err;
 
+	if (!nbytes)
+		return 0;
+
+	sg = req->src;
+	offset = sg->offset;
+
 	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
 		void *data;
 
-- 
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 1/2] crypto: skcipher - noop for enc/dec with NULL data

[Stephan Müller]

Am Montag, 9. Oktober 2017, 16:19:25 CEST schrieb Herbert Xu:

Hi Herbert,

> On Sat, Oct 07, 2017 at 02:56:24PM +0200, Stephan Müller wrote:
> > Though, this opens up the shash issue I tried to fix.
> 
> Does this patch fix the crash?

I get the following during boot:

[    1.042673] ------------[ cut here ]------------
[    1.043208] kernel BUG at crypto/asymmetric_keys/public_key.c:96!
[    1.044235] invalid opcode: 0000 [#1] SMP
[    1.044661] Modules linked in:
[    1.044964] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 4.14.0-rc1+ #556
[    1.045638] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[    1.046397] task: ffff8ecefc880000 task.stack: ffffa776c031c000
[    1.046943] RIP: 0010:public_key_verify_signature+0x25c/0x270
[    1.047539] RSP: 0018:ffffa776c031fcd8 EFLAGS: 00010246
[    1.047997] RAX: ffffffffa5cb1d5f RBX: ffff8eceb61f9780 RCX: 
0000000000000000
[    1.048618] RDX: ffff8eceb6169cc0 RSI: ffff8eceb6169cc0 RDI: 
ffff8ecefc99be80
[    1.049261] RBP: ffffa776c031fcf0 R08: 00000000000000c9 R09: 
ffff8ecefd003800
[    1.049870] R10: ffffc78580d88100 R11: 7fffffffffffffff R12: 
0000000000000001
[    1.050487] R13: ffff8eceb61fa2a0 R14: ffffffffa60c5870 R15: 
0000000000000542
[    1.051118] FS:  0000000000000000(0000) GS:ffff8eceffd00000(0000) knlGS:
0000000000000000
[    1.051805] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.052295] CR2: 00007f89a5ee2000 CR3: 0000000036036004 CR4: 
00000000003606e0
[    1.052853] Call Trace:
[    1.053069]  ? x509_check_for_self_signed+0x86/0xd0
[    1.053446]  x509_cert_parse+0x15e/0x1c0
[    1.053764]  x509_key_preparse+0x26/0x1e0
[    1.054094]  asymmetric_key_preparse+0x5c/0xd0
[    1.054438]  key_create_or_update+0x137/0x430
[    1.054789]  ? set_debug_rodata+0x17/0x17
[    1.055119]  load_system_certificate_list+0x99/0xfa
[    1.055494]  ? system_trusted_keyring_init+0x66/0x66
[    1.055890]  ? set_debug_rodata+0x17/0x17
[    1.056221]  do_one_initcall+0x41/0x160
[    1.056519]  kernel_init_freeable+0x173/0x201
[    1.056867]  ? rest_init+0xb0/0xb0
[    1.057161]  kernel_init+0xe/0x110
[    1.057426]  ret_from_fork+0x25/0x30
[    1.057714] Code: 40 00 85 c0 b8 7f ff ff ff 44 0f 45 f8 eb 8a 48 8d bd e0 
fe ff ff e8 d4 ad 40 00 44 8b bd 00 ff ff ff e9 5a ff ff ff 0f 0b 0f 0b <0f> 
0b 0f 0b 41 bf ea ff ff ff e9 7a ff ff ff 0f 1f 44 00 00 0f 
[    1.059208] RIP: public_key_verify_signature+0x25c/0x270 RSP: 
ffffa776c031fcd8
[    1.059782] ---[ end trace 5363a8b61ab8b581 ]---
[    1.060236] Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b
[    1.060236] 
[    1.061073] Kernel Offset: 0x24000000 from 0xffffffff81000000 (relocation 
range: 0xffffffff80000000-0xffffffffbfffffff)
[    1.061990] ---[ end Kernel panic - not syncing: Attempted to kill init! 
exitcode=0x0000000b
Ciao
Stephan

[PATCH v2] crypto: shash - Fix zero-length shash ahash digest crash

[Herbert Xu]

On Mon, Oct 09, 2017 at 05:13:48PM +0200, Stephan Müller wrote:
> Am Montag, 9. Oktober 2017, 16:19:25 CEST schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Sat, Oct 07, 2017 at 02:56:24PM +0200, Stephan Müller wrote:
> > > Though, this opens up the shash issue I tried to fix.
> > 
> > Does this patch fix the crash?
> 
> I get the following during boot:

Thanks for the quick response.  Obviously not doing the hash
at all isn't right.

Does this patch work better?

---8<---
The shash ahash digest adaptor function may crash if given a
zero-length input together with a null SG list.  This is because
it tries to read the SG list before looking at the length.

This patch fixes it by checking the length first.

Cc: <stable@vger.kernel.org>
Reported-by: Stephan Müller<smueller@chronox.de>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/crypto/shash.c b/crypto/shash.c
index 8fcecc6..325a14d 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -275,12 +275,14 @@ static int shash_async_finup(struct ahash_request *req)
 
 int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
 {
-	struct scatterlist *sg = req->src;
-	unsigned int offset = sg->offset;
 	unsigned int nbytes = req->nbytes;
+	struct scatterlist *sg;
+	unsigned int offset;
 	int err;
 
-	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
+	if (nbytes &&
+	    (sg = req->src, offset = sg->offset,
+	     nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset))) {
 		void *data;
 
 		data = kmap_atomic(sg_page(sg));
-- 
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 v2] crypto: shash - Fix zero-length shash ahash digest crash

[Stephan Müller]

Am Montag, 9. Oktober 2017, 17:30:02 CEST schrieb Herbert Xu:

Hi Herbert,

> The shash ahash digest adaptor function may crash if given a
> zero-length input together with a null SG list.  This is because
> it tries to read the SG list before looking at the length.
> 
> This patch fixes it by checking the length first.

The patch fixes the issue.
> 
> Cc: <stable@vger.kernel.org>
> Reported-by: Stephan Müller<smueller@chronox.de>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Tested-by: Stephan Müller <smueller@chronox.de>

Ciao
Stephan

[PATCH 2/2] crypto: shash - no kmap of zero SG

[Stephan Müller]

In case the caller provides an SG with zero data, prevent a kmap of the
page pointed to by the SG. In this case, it is possible that the page
does not exist.

This fixes a crash in authenc() when the plaintext is zero and thus the
encryption operation is a noop. In this case, no input data exists that
can be hashed. The crash is triggerable via AF_ALG from unprivileged
user space.

Fixes: 3b2f6df08258e ("crypto: hash - Export shash through ahash")
CC: Herbert Xu <herbert@gondor.apana.org.au>
CC: <stable@vger.kernel.org>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/shash.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/shash.c b/crypto/shash.c
index 5e31c8d776df..32d0e1806bf4 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -278,9 +278,11 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
 	struct scatterlist *sg = req->src;
 	unsigned int offset = sg->offset;
 	unsigned int nbytes = req->nbytes;
+	unsigned int process = min(sg->length,
+				   ((unsigned int)(PAGE_SIZE)) - offset);
 	int err;
 
-	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
+	if (process && nbytes < process) {
 		void *data;
 
 		data = kmap_atomic(sg_page(sg));
-- 
2.13.5

Re: [PATCH 2/2] crypto: shash - no kmap of zero SG

[Herbert Xu]

On Sun, Sep 24, 2017 at 08:25:17AM +0200, Stephan Müller wrote:
> In case the caller provides an SG with zero data, prevent a kmap of the
> page pointed to by the SG. In this case, it is possible that the page
> does not exist.
> 
> This fixes a crash in authenc() when the plaintext is zero and thus the
> encryption operation is a noop. In this case, no input data exists that
> can be hashed. The crash is triggerable via AF_ALG from unprivileged
> user space.
> 
> Fixes: 3b2f6df08258e ("crypto: hash - Export shash through ahash")
> CC: Herbert Xu <herbert@gondor.apana.org.au>
> CC: <stable@vger.kernel.org>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
>  crypto/shash.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/shash.c b/crypto/shash.c
> index 5e31c8d776df..32d0e1806bf4 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -278,9 +278,11 @@ int shash_ahash_digest(struct ahash_request *req, struct shash_desc *desc)
>  	struct scatterlist *sg = req->src;
>  	unsigned int offset = sg->offset;
>  	unsigned int nbytes = req->nbytes;
> +	unsigned int process = min(sg->length,
> +				   ((unsigned int)(PAGE_SIZE)) - offset);
>  	int err;
>  
> -	if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
> +	if (process && nbytes < process) {

Sorry but your patch makes no sense.  The only difference between
your patch and the status quo is when process == zero.  However,
if process is zero then the if condition cannot hold since it's
an unsigned comparison.  So how can this patch make any difference
at all?

Cheers,
-- 
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 0/2] fix authenc() kernel crash

[Stephan Müller]

Am Sonntag, 24. September 2017, 08:22:54 CEST schrieb Stephan Müller:

Hi Herbert,

(private)

> Hi Herbert,
> 
> The two patches together fix a kernel crash that can be triggered via
> AF_ALG when using authenc() with zero plaintext.
> 
> The changes are also tested to verify that the hashing on null data
> is still supported.

You can test / verify the patch with the HEAD code of libkcapi found in [1].

Please execute:

./configure --enable-kcapi-test
make
bin/kcapi -h -x 2 -c "authenc(hmac(sha256),cbc(aes))" -d 2

Note, the last command may take a couple of seconds.

[1] https://github.com/smuellerDD/libkcapi


Ciao
Stephan