* [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()
@ 2023-12-11 13:59 Shigeru Yoshida
2023-12-22 3:42 ` Herbert Xu
0 siblings, 1 reply; 6+ messages in thread
From: Shigeru Yoshida @ 2023-12-11 13:59 UTC (permalink / raw)
To: herbert, davem; +Cc: dhowells, linux-crypto, linux-kernel, Shigeru Yoshida
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(+)
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;
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()
2023-12-11 13:59 [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg() Shigeru Yoshida
@ 2023-12-22 3:42 ` Herbert Xu
2023-12-27 4:03 ` Shigeru Yoshida
2024-01-03 15:36 ` David Howells
0 siblings, 2 replies; 6+ messages in thread
From: Herbert Xu @ 2023-12-22 3:42 UTC (permalink / raw)
To: Shigeru Yoshida; +Cc: davem, dhowells, linux-crypto, linux-kernel
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,
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()
2023-12-22 3:42 ` Herbert Xu
@ 2023-12-27 4:03 ` Shigeru Yoshida
2024-01-03 15:36 ` David Howells
1 sibling, 0 replies; 6+ messages in thread
From: Shigeru Yoshida @ 2023-12-27 4:03 UTC (permalink / raw)
To: herbert; +Cc: davem, dhowells, linux-crypto, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()
@ 2024-01-03 3:05 xingwei lee
0 siblings, 0 replies; 6+ messages in thread
From: xingwei lee @ 2024-01-03 3:05 UTC (permalink / raw)
To: syoshida; +Cc: davem, dhowells, herbert, linux-crypto, linux-kernel
Hi,Shigeru and Herbert. Happy New Year anyway.
I also found this bug and tried to reproduce it.
My own syzkaller crashes titled "double-free in af_alg_free_sg” or
“KASAN: use-after-free in af_alg_free_sg” lead me to consider it maybe
a security-related problem.
I reproduced it with repro.c and repro.txt and also bisection to this
commit: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/crypto/algif_hash.c?id=b6d972f6898308fbe7e693bf8d44ebfdb1cd2dc4
=* repro.c =*
// autogenerated by syzkaller (https://github.com/google/syzkaller)
#define _GNU_SOURCE
#include <endian.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>
uint64_t r[3] = {0xffffffffffffffff, 0xffffffffffffffff, 0xffffffffffffffff};
int main(void) {
syscall(__NR_mmap, /*addr=*/0x1ffff000ul, /*len=*/0x1000ul, /*prot=*/0ul,
/*flags=*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
syscall(__NR_mmap, /*addr=*/0x20000000ul, /*len=*/0x1000000ul, /*prot=*/7ul,
/*flags=*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
syscall(__NR_mmap, /*addr=*/0x21000000ul, /*len=*/0x1000ul, /*prot=*/0ul,
/*flags=*/0x32ul, /*fd=*/-1, /*offset=*/0ul);
intptr_t res = 0;
res = syscall(__NR_socket, /*domain=*/0x26ul, /*type=*/5ul, /*proto=*/0);
if (res != -1) r[0] = res;
*(uint16_t*)0x20000040 = 0x26;
memcpy((void*)0x20000042, "hash\000\000\000\000\000\000\000\000\000\000", 14);
*(uint32_t*)0x20000050 = 0;
*(uint32_t*)0x20000054 = 0;
memcpy((void*)0x20000058,
"poly1305\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
"\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
"\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
"\000\000\000\000\000\000\000",
64);
syscall(__NR_bind, /*fd=*/r[0], /*addr=*/0x20000040ul, /*addrlen=*/0x58ul);
res = syscall(__NR_accept4, /*fd=*/r[0], /*peer=*/0ul, /*peerlen=*/0ul,
/*flags=*/0ul);
if (res != -1) r[1] = res;
*(uint64_t*)0x20000d80 = 0;
*(uint32_t*)0x20000d88 = 0;
*(uint64_t*)0x20000d90 = 0x20000d40;
*(uint64_t*)0x20000d40 = 0x20000d00;
*(uint16_t*)0x20000d00 = 0;
*(uint64_t*)0x20000d48 = 0x14;
*(uint64_t*)0x20000d98 = 1;
*(uint64_t*)0x20000da0 = 0;
*(uint64_t*)0x20000da8 = 0;
*(uint32_t*)0x20000db0 = 0;
syscall(__NR_sendmsg, /*fd=*/r[1], /*msg=*/0x20000d80ul, /*f=*/0x400c000ul);
res = syscall(__NR_accept4, /*fd=*/r[1], /*peer=*/0ul, /*peerlen=*/0ul,
/*flags=*/0ul);
if (res != -1) r[2] = res;
*(uint64_t*)0x20000840 = 0;
*(uint32_t*)0x20000848 = 0;
*(uint64_t*)0x20000850 = 0;
*(uint64_t*)0x20000858 = 0;
*(uint64_t*)0x20000860 = 0;
*(uint64_t*)0x20000868 = 0;
*(uint32_t*)0x20000870 = 0x4000;
syscall(__NR_sendmsg, /*fd=*/r[2], /*msg=*/0x20000840ul, /*f=*/0x4001ul);
return 0;
}
=* repro.txt =*
r0 = socket$alg(0x26, 0x5, 0x0)
bind$alg(r0, &(0x7f0000000040)={0x26, 'hash\x00', 0x0, 0x0,
'poly1305\x00'}, 0x58)
r1 = accept4(r0, 0x0, 0x0, 0x0)
sendmsg$BATADV_CMD_SET_HARDIF(r1, &(0x7f0000000d80)={0x0, 0x0,
&(0x7f0000000d40)={&(0x7f0000000d00)=ANY=[@ANYBLOB, @ANYRES16=0x0,
@ANYBLOB], 0x14}}, 0x400c000)
r2 = accept4(r1, 0x0, 0x0, 0x0)
sendmsg$alg(r2, &(0x7f0000000840)={0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x4000}, 0x4001)
After analysing the uninitialized of ctx->sgl, it may cause (without
KMSAN in linux kernel)
void af_alg_free_sg(struct af_alg_sgl *sgl)
{
int i;
if (sgl->sgt.sgl) {
if (sgl->need_unpin)
for (i = 0; i < sgl->sgt.nents; i++)
unpin_user_page(sg_page(&sgl->sgt.sgl[i]));
if (sgl->sgt.sgl != sgl->sgl)
kvfree(sgl->sgt.sgl);
sgl->sgt.sgl = NULL;
}
}
1. If sgl->sgt.sgl is 0x0, the poc triggers nothing
2. If sgl->sgt.sgl is not null but like 0xbbbbbbbbbbbbbbbb,
unpin_user_page will crash like “wild-memory access”.
3. If sgl->sgt.sgl happens to be a pointer whether it is being used or
released, sgl->sgt.nents<0, kvfree can definitely cause uaf or double
free and maybe lead to control flow hijacking.
The incorrect logic of unlock_free label can really cause security issues.
I hope the reproducer and analysis helps.
Best regards.
xingwei Lee
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()
2023-12-22 3:42 ` Herbert Xu
2023-12-27 4:03 ` Shigeru Yoshida
@ 2024-01-03 15:36 ` David Howells
2024-01-04 2:03 ` Herbert Xu
1 sibling, 1 reply; 6+ messages in thread
From: David Howells @ 2024-01-03 15:36 UTC (permalink / raw)
To: Herbert Xu; +Cc: dhowells, Shigeru Yoshida, davem, linux-crypto, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg()
2024-01-03 15:36 ` David Howells
@ 2024-01-04 2:03 ` Herbert Xu
0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2024-01-04 2:03 UTC (permalink / raw)
To: David Howells; +Cc: Shigeru Yoshida, davem, linux-crypto, linux-kernel
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,
--
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-04 2:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-11 13:59 [PATCH] crypto: af_alg/hash: Fix uninit-value access in af_alg_free_sg() Shigeru Yoshida
2023-12-22 3:42 ` Herbert Xu
2023-12-27 4:03 ` Shigeru Yoshida
2024-01-03 15:36 ` David Howells
2024-01-04 2:03 ` Herbert Xu
-- strict thread matches above, loose matches on Subject: below --
2024-01-03 3:05 xingwei lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox