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 <[email protected]>
---
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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
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
Herbert Xu <[email protected]> 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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt