crypto: scomp - Fix wild memory accesses in scomp_free_streams().

PATCH New Kuniyuki Iwashima 2 participants 4 messages Apr 10, 2025 – Apr 11, 2025 Crypto API

lore

Share & tools

lore

Kuniyuki Iwashima

author

2025-04-10 18:34 UTC #0 lore raw

PATCH 1/1 crypto: scomp - Fix wild memory accesses in scomp_free_streams().

syzkaller reported the splat below [0].

When alg->alloc_ctx() fails, alg is passed to scomp_free_streams(), but alg->stream is still NULL there.

Also, ps->ctx has ERR_PTR(), which would bypass the NULL check and could be passed to alg->free_ctx(ps->ctx).

Let's fix the two wild memory accesses.

[0]:

BUG: unable to handle page fault for address: ffe21c0032422608
 PF: supervisor read access in kernel mode
 PF: error_code(0x0000) - not-present page
PGD 13fef4067 P4D 13fef3067 PUD 13fef2067 PMD 0
Oops: Oops: 0000 [#1] SMP KASAN
CPU: 0 UID: 0 PID: 316 Comm: syz-executor483 Not tainted 6.14.0-13344-ga9843689e2de #28 PREEMPT(voluntary)  167b7ecb8f281ed56016416cdf1d8bb342db88fc
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
RIP: 0010:scomp_free_streams crypto/scompress.c:117 [inline]
RIP: 0010:scomp_alloc_streams crypto/scompress.c:140 [inline]
RIP: 0010:crypto_scomp_init_tfm+0x5b0/0x7e0 crypto/scompress.c:158
Code: 74 08 48 89 df e8 90 74 8a ff 48 8b 03 48 8b 0c 24 48 8d 1c 08 48 83 c3 40 48 89 d8 48 c1 e8 03 48 b9 00 00 00 00 00 fc ff df <80> 3c 08 00 74 08 48 89 df e8 62 74 8a ff 48 8b 1b 48 85 db 0f 84
RSP: 0018:ffa000000154f028 EFLAGS: 00010a02
RAX: 1fe2200032422608 RBX: ff11000192113040 RCX: dffffc0000000000
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 1ffffffff0c0463f R08: ffffffff872f7bb7 R09: 1ffffffff0e5ef76
R10: dffffc0000000000 R11: fffffbfff0e5ef77 R12: 0000000000000000
R13: ffffffff86c93028 R14: ffd1ffffffc0ebf8 R15: 1ffa3ffffff81d7f
FS:  00007f35c192a740(0000) GS:ff11000192113000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffe21c0032422608 CR3: 00000001062f0003 CR4: 0000000000771ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 <TASK>
 crypto_create_tfm_node+0x163/0x3b0 crypto/api.c:532
 crypto_create_tfm crypto/internal.h:136 [inline]
 crypto_init_scomp_ops_async+0x59/0x120 crypto/scompress.c:338
 crypto_create_tfm_node+0x163/0x3b0 crypto/api.c:532
 crypto_alloc_tfm_node+0x172/0x3e0 crypto/api.c:633
 ipcomp_init_state+0x14d/0x2a0 net/xfrm/xfrm_ipcomp.c:345
 ipcomp4_init_state+0xcc/0x9f0 net/ipv4/ipcomp.c:137
 __xfrm_init_state+0xa74/0x13d0 net/xfrm/xfrm_state.c:3178
 xfrm_state_construct net/xfrm/xfrm_user.c:922 [inline]
 xfrm_add_sa+0x2f11/0x4000 net/xfrm/xfrm_user.c:986
 xfrm_user_rcv_msg+0x61d/0x890 net/xfrm/xfrm_user.c:3459
 netlink_rcv_skb+0x204/0x470 net/netlink/af_netlink.c:2534
 xfrm_netlink_rcv+0x79/0x90 net/xfrm/xfrm_user.c:3481
 netlink_unicast_kernel net/netlink/af_netlink.c:1313 [inline]
 netlink_unicast+0x754/0x8c0 net/netlink/af_netlink.c:1339
 netlink_sendmsg+0x800/0xb20 net/netlink/af_netlink.c:1883
 sock_sendmsg_nosec net/socket.c:712 [inline]
 __sock_sendmsg net/socket.c:727 [inline]
 ____sys_sendmsg+0x5ee/0x960 net/socket.c:2566
 ___sys_sendmsg+0x1e6/0x260 net/socket.c:2620
 __sys_sendmsg net/socket.c:2652 [inline]
 __do_sys_sendmsg net/socket.c:2657 [inline]
 __se_sys_sendmsg net/socket.c:2655 [inline]
 __x64_sys_sendmsg+0x18a/0x250 net/socket.c:2655
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0xe4/0x1c0 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x4b/0x53
RIP: 0033:0x7f35c196be5d
Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 73 9f 1b 00 f7 d8 64 89 01 48
RSP: 002b:00007fffd68e23a8 EFLAGS: 00000206 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f35c196be5d
RDX: 0000000000000800 RSI: 0000200000000000 RDI: 0000000000000003
RBP: 0000000000000004 R08: 00007fffd68e2146 R09: 0000004000000000
R10: 0000000000000000 R11: 0000000000000206 R12: 431bde82d7b634db
R13: 00007fffd68e23d0 R14: 0000000000403df0 R15: 00007f35c1b76000
 </TASK>
Modules linked in:

CR2: ffe21c0032422608

Fixes 3d72ad46a23a ("crypto: acomp - Move stream management into scomp layer")

Reported-by syzkaller <[email protected]>

Signed-off-by Kuniyuki Iwashima <[email protected]>

---
 crypto/scompress.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

crypto/scompress.c +17 -7

index f67ce38d203d..f00ab9720454 100644
@@ -106,14 +106,11 @@ static int crypto_scomp_alloc_scratches(void)
 	return -ENOMEM;
 }
 
-static void scomp_free_streams(struct scomp_alg *alg)
+static void __scomp_free_streams(struct scomp_alg *alg,
+				 struct crypto_acomp_stream __percpu *stream)
 {
-	struct crypto_acomp_stream __percpu *stream = alg->stream;
 	int i;
 
-	if (!stream)
-		return;
-
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);

@@ -126,6 +123,16 @@ static void scomp_free_streams(struct scomp_alg *alg)
 	free_percpu(stream);
 }
 
+static void scomp_free_streams(struct scomp_alg *alg)
+{
+	struct crypto_acomp_stream __percpu *stream = alg->stream;
+
+	if (!stream)
+		return;
+
+	__scomp_free_streams(alg, stream);
+}
+
 static int scomp_alloc_streams(struct scomp_alg *alg)
 {
 	struct crypto_acomp_stream __percpu *stream;

@@ -140,8 +147,11 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
 
 		ps->ctx = alg->alloc_ctx();
 		if (IS_ERR(ps->ctx)) {
-			scomp_free_streams(alg);
-			return PTR_ERR(ps->ctx);
+			int err = PTR_ERR(ps->ctx);
+
+			ps->ctx = NULL;
+			__scomp_free_streams(alg, stream);
+			return err;
 		}
 
 		spin_lock_init(&ps->lock);

Signature · 1 line

2.49.0

Herbert Xu

maintainer

2025-04-11 01:26 UTC #1 lore raw

PATCH v2 1/1 crypto: scomp - Fix wild memory accesses in scomp_free_streams

Quoted 7 lines · Kuniyuki Iwashima

syzkaller reported the splat below [0].
When alg->alloc_ctx() fails, alg is passed to scomp_free_streams(), but alg->stream is still NULL there.
Also, ps->ctx has ERR_PTR(), which would bypass the NULL check and could be passed to alg->free_ctx(ps->ctx).

Thanks for the report. I think we should instead move the assignment up and test for IS_ERR_OR_NULL in scomp_free_streams.

---8<---
In order to use scomp_free_streams to free the partially allocted streams in the allocation error path, move the alg->stream assignment to the beginning. Also check for error pointers in scomp_free_streams before freeing the ctx.

Finally set alg->stream to NULL to not break subsequent attempts to allocate the streams.

Fixes 3d72ad46a23a ("crypto: acomp - Move stream management into scomp layer")

Reported-by syzkaller <[email protected]>

Co-developed-by Kuniyuki Iwashima <[email protected]>

Signed-off-by Kuniyuki Iwashima <[email protected]>

Co-developed-by Herbert Xu <[email protected]>

Signed-off-by Herbert Xu <[email protected]>

crypto/scompress.c +4 -3

index f67ce38d203d..5762fcc63b51 100644
@@ -111,13 +111,14 @@ static void scomp_free_streams(struct scomp_alg *alg)
 	struct crypto_acomp_stream __percpu *stream = alg->stream;
 	int i;
 
+	alg->stream = NULL;
 	if (!stream)
 		return;
 
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);
 
-		if (!ps->ctx)
+		if (IS_ERR_OR_NULL(ps->ctx))
 			break;
 
 		alg->free_ctx(ps->ctx);

@@ -135,6 +136,8 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
 	if (!stream)
 		return -ENOMEM;
 
+	alg->stream = stream;
+
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);

@@ -146,8 +149,6 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
 
 		spin_lock_init(&ps->lock);
 	}
-
-	alg->stream = stream;
 	return 0;
 }

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Kuniyuki Iwashima

author

2025-04-11 02:44 UTC #2 lore raw

Re: [v2 PATCH] crypto: scomp - Fix wild memory accesses in scomp_free_streams

From: Herbert Xu <[email protected]>
Date: Fri, 11 Apr 2025 09:26:47 +0800

Quoted 11 lines · Kuniyuki Iwashima

Quoted 7 lines
syzkaller reported the splat below [0].
When alg->alloc_ctx() fails, alg is passed to scomp_free_streams(), but alg->stream is still NULL there.
Also, ps->ctx has ERR_PTR(), which would bypass the NULL check and could be passed to alg->free_ctx(ps->ctx).
Thanks for the report. I think we should instead move the assignment up and test for IS_ERR_OR_NULL in scomp_free_streams.

I didn't move the assignment just because I was not sure if the immature alg->stream could be accessed by another thread.

But if it's okay, v2 looks better to me.

Thanks!

Quoted 56 lines

---8<---
In order to use scomp_free_streams to free the partially allocted streams in the allocation error path, move the alg->stream assignment to the beginning. Also check for error pointers in scomp_free_streams before freeing the ctx.
Finally set alg->stream to NULL to not break subsequent attempts to allocate the streams.
Fixes 3d72ad46a23a ("crypto: acomp - Move stream management into scomp layer")
Reported-by syzkaller <[email protected]>
Co-developed-by Kuniyuki Iwashima <[email protected]>
Signed-off-by Kuniyuki Iwashima <[email protected]>
Co-developed-by Herbert Xu <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
crypto/scompress.c +4 -3
index f67ce38d203d..5762fcc63b51 100644
@@ -111,13 +111,14 @@ static void scomp_free_streams(struct scomp_alg *alg)
 	struct crypto_acomp_stream __percpu *stream = alg->stream;
 	int i;
 
+	alg->stream = NULL;
 	if (!stream)
 		return;
 
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);
 
-		if (!ps->ctx)
+		if (IS_ERR_OR_NULL(ps->ctx))
 			break;
 
 		alg->free_ctx(ps->ctx);
@@ -135,6 +136,8 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
 	if (!stream)
 		return -ENOMEM;
 
+	alg->stream = stream;
+
 	for_each_possible_cpu(i) {
 		struct crypto_acomp_stream *ps = per_cpu_ptr(stream, i);
 
@@ -146,8 +149,6 @@ static int scomp_alloc_streams(struct scomp_alg *alg)
 
 		spin_lock_init(&ps->lock);
 	}
-
-	alg->stream = stream;
 	return 0;
 }
 

Herbert Xu

maintainer

2025-04-11 02:51 UTC #3 lore raw

Re: [v2 PATCH] crypto: scomp - Fix wild memory accesses in scomp_free_streams

Quoted 3 lines · Kuniyuki Iwashima

I didn't move the assignment just because I was not sure if the immature alg->stream could be accessed by another thread.

The initialisation is conducted under the scomp lock which is taken by everyone in init_tfm, so until the allocation completes and the lock is dropped no other init_tfm can see the assigned value.

As to the actual users of alg->stream, it must have first passed through a successful init_tfm call, again guaranteeing that the value of alg->stream has been initialised.

Quoted 1 line

But if it's okay, v2 looks better to me.

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt