diff mbox series

crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()

Message ID 20231211135949.689204-1-syoshida@redhat.com
State New
Headers
Series crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg() |

Commit Message

Shigeru Yoshida Dec. 11, 2023, 1:59 p.m. UTC 
  KMSAN reported the following uninit-value access issue:

=====================================================
BUG: KMSAN: uninit-value in af_alg_free_sg+0x1c1/0x270 crypto/af_alg.c:547
 af_alg_free_sg+0x1c1/0x270 crypto/af_alg.c:547
 hash_sendmsg+0x188f/0x1ce0 crypto/algif_hash.c:172
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 ____sys_sendmsg+0x997/0xd60 net/socket.c:2584
 ___sys_sendmsg+0x271/0x3b0 net/socket.c:2638
 __sys_sendmsg net/socket.c:2667 [inline]
 __do_sys_sendmsg net/socket.c:2676 [inline]
 __se_sys_sendmsg net/socket.c:2674 [inline]
 __x64_sys_sendmsg+0x2fa/0x4a0 net/socket.c:2674
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

Uninit was created at:
 slab_post_alloc_hook+0x103/0x9e0 mm/slab.h:768
 slab_alloc_node mm/slub.c:3478 [inline]
 __kmem_cache_alloc_node+0x5d5/0x9b0 mm/slub.c:3517
 __do_kmalloc_node mm/slab_common.c:1006 [inline]
 __kmalloc+0x118/0x410 mm/slab_common.c:1020
 kmalloc include/linux/slab.h:604 [inline]
 sock_kmalloc+0x104/0x1a0 net/core/sock.c:2681
 hash_accept_parent_nokey crypto/algif_hash.c:418 [inline]
 hash_accept_parent+0xbc/0x470 crypto/algif_hash.c:445
 af_alg_accept+0x1d8/0x810 crypto/af_alg.c:439
 hash_accept+0x368/0x800 crypto/algif_hash.c:254
 do_accept+0x803/0xa70 net/socket.c:1927
 __sys_accept4_file net/socket.c:1967 [inline]
 __sys_accept4+0x170/0x340 net/socket.c:1997
 __do_sys_accept4 net/socket.c:2008 [inline]
 __se_sys_accept4 net/socket.c:2005 [inline]
 __x64_sys_accept4+0xc0/0x150 net/socket.c:2005
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

CPU: 0 PID: 14168 Comm: syz-executor.2 Not tainted 6.7.0-rc4-00009-gbee0e7762ad2 #13
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014
=====================================================

In hash_sendmsg(), hash_alloc_result() may fail and return -ENOMEM if
sock_kmalloc() fails. In this case, hash_sendmsg() jumps to the unlock_free
label and calls af_alg_free_sg() with ctx->sgl.sgt.sgl uninitialized. This
causes the above uninit-value access issue for ctx->sgl.sgt.sgl.

This patch fixes this issue by initializing ctx->sgl.sgt.sgl when the
structure is allocated in hash_accept_parent_nokey().

Fixes: c662b043cdca ("crypto: af_alg/hash: Support MSG_SPLICE_PAGES")
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 crypto/algif_hash.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Herbert Xu Dec. 22, 2023, 3:42 a.m. UTC | #1 
On Mon, Dec 11, 2023 at 10:59:49PM +0900, Shigeru Yoshida wrote:
>
> Fixes: c662b043cdca ("crypto: af_alg/hash: Support MSG_SPLICE_PAGES")

I think it should actually be

	b6d972f6898308fbe7e693bf8d44ebfdb1cd2dc4
	crypto: af_alg/hash: Fix recvmsg() after sendmsg(MSG_MORE)

Anyway, I think we should fix it by adding a new goto label that
does not free the SG list:

unlock_free:
	af_alg_free_sg(&ctx->sgl);
<--- Add new label here
	hash_free_result(sk, ctx);
	ctx->more = false;
	goto unlock;

Thanks,
Shigeru Yoshida Dec. 27, 2023, 4:03 a.m. UTC | #2 
On Fri, 22 Dec 2023 11:42:43 +0800, Herbert Xu wrote:
> On Mon, Dec 11, 2023 at 10:59:49PM +0900, Shigeru Yoshida wrote:
>>
>> Fixes: c662b043cdca ("crypto: af_alg/hash: Support MSG_SPLICE_PAGES")
> 
> I think it should actually be
> 
> 	b6d972f6898308fbe7e693bf8d44ebfdb1cd2dc4
> 	crypto: af_alg/hash: Fix recvmsg() after sendmsg(MSG_MORE)
> 
> Anyway, I think we should fix it by adding a new goto label that
> does not free the SG list:
> 
> unlock_free:
> 	af_alg_free_sg(&ctx->sgl);
> <--- Add new label here
> 	hash_free_result(sk, ctx);
> 	ctx->more = false;
> 	goto unlock;

Thanks for the feedback, and sorry for the late response.

I'll check the code again, and send v2 patch.

Thanks,
Shigeru

> 
> 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
>
David Howells Jan. 3, 2024, 3:36 p.m. UTC | #3 
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Anyway, I think we should fix it by adding a new goto label that
> does not free the SG list:
> 
> unlock_free:
> 	af_alg_free_sg(&ctx->sgl);
> <--- Add new label here
> 	hash_free_result(sk, ctx);
> 	ctx->more = false;
> 	goto unlock;

Hmmm...  Is that going to get you a potential memory leak?

ctx->sgl.sgt.sgl could (in theory) point to an allocated table.  I guess that
would be cleaned up by af_alg_free_areq_sgls(), so there's probably no leak
there.

OTOH, af_alg_free_areq_sgls() is going to call af_alg_free_sg(), so maybe we
want to initialise sgl->sgt.sgl to NULL as well.

Does it make sense just to clear *ctx entirely in hash_accept_parent_nokey()?

David
Herbert Xu Jan. 4, 2024, 2:03 a.m. UTC | #4 
On Wed, Jan 03, 2024 at 03:36:51PM +0000, David Howells wrote:
> Hmmm...  Is that going to get you a potential memory leak?
> 
> ctx->sgl.sgt.sgl could (in theory) point to an allocated table.  I guess that
> would be cleaned up by af_alg_free_areq_sgls(), so there's probably no leak
> there.

The SG list is only setup in this function, and gets freed before
we return.  There should be no SG list on entry.  It's only because
you added the special case for a zero-length hash that we hit the
bogus free.  So we should fix this by not freeing the SG list in
the zero-length case, as it was never allocated.

> OTOH, af_alg_free_areq_sgls() is going to call af_alg_free_sg(), so maybe we
> want to initialise sgl->sgt.sgl to NULL as well.

That has nothing to do with this.  This SG list is specific to
algif_hash and has nothing to do with the shared SG list used
by aead and skcipher.

Cheers,
diff mbox series

Patch

diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c
index 82c44d4899b9..a51b58d36d60 100644
--- a/crypto/algif_hash.c
+++ b/crypto/algif_hash.c
@@ -419,6 +419,7 @@  static int hash_accept_parent_nokey(void *private, struct sock *sk)
 	if (!ctx)
 		return -ENOMEM;
 
+	ctx->sgl.sgt.sgl = NULL;
 	ctx->result = NULL;
 	ctx->len = len;
 	ctx->more = false;