Skip to contentSkip to conversation

crypto: fix uninit-value in af_alg_free_resources

PATCH Superseded Pavel Skripkin 3 participants 6 messages Aug 13, 2023 – Aug 18, 2023 Crypto API

lore
Share & tools
lore

Pavel Skripkin

author
#0 lore raw

PATCH 1/1 crypto: fix uninit-value in af_alg_free_resources

Syzbot was able to trigger use of uninitialized memory in af_alg_free_resources.

Bug is caused by missing initialization of rsgl->sgl.need_unpin before adding to rsgl_list. Then in case of extract_iter_to_sg() failure, rsgl is left with uninitialized need_unpin which is read during clean up

BUG: KMSAN: uninit-value in af_alg_free_sg crypto/af_alg.c:545 [inline]
BUG: KMSAN: uninit-value in af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
BUG: KMSAN: uninit-value in af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 af_alg_free_sg crypto/af_alg.c:545 [inline]
 af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
 af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 _skcipher_recvmsg crypto/algif_skcipher.c:144 [inline]

...

Uninit was created at:

 slab_post_alloc_hook+0x12f/0xb70 mm/slab.h:767
 slab_alloc_node mm/slub.c:3470 [inline]
 __kmem_cache_alloc_node+0x536/0x8d0 mm/slub.c:3509
 __do_kmalloc_node mm/slab_common.c:984 [inline]
 __kmalloc+0x121/0x3c0 mm/slab_common.c:998
 kmalloc include/linux/slab.h:586 [inline]
 sock_kmalloc+0x128/0x1c0 net/core/sock.c:2683
 af_alg_alloc_areq+0x41/0x2a0 crypto/af_alg.c:1188
 _skcipher_recvmsg crypto/algif_skcipher.c:71 [inline]
Fixes c1abe6f570af ("crypto: af_alg: Use extract_iter_to_sg() to create scatterlists")
Reported-and-tested-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=cba21d50095623218389
Signed-off-by Pavel Skripkin <[email protected]>
---
 crypto/af_alg.c | 2 ++
 1 file changed, 2 insertions(+)
crypto/af_alg.c +2 -0
index 06b15b9f661c..4e4f2d837481 100644
@@ -1241,6 +1241,8 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags,
 				return -ENOMEM;
 		}
 
+		rsgl->sgl.need_unpin = 0;
+
 		rsgl->sgl.sgt.sgl = rsgl->sgl.sgl;
 		rsgl->sgl.sgt.nents = 0;
 		rsgl->sgl.sgt.orig_nents = 0;
Signature · 1 line
2.40.1
#1 lore raw
Quoted 6 lines · Pavel Skripkin

Syzbot was able to trigger use of uninitialized memory in af_alg_free_resources.

Bug is caused by missing initialization of rsgl->sgl.need_unpin before adding to rsgl_list. Then in case of extract_iter_to_sg() failure, rsgl is left with uninitialized need_unpin which is read during clean up

Looks feasible :-).

crypto/af_alg.c int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags, · line 1245

Quoted 2 lines
crypto/af_alg.c +2 -0
+		rsgl->sgl.need_unpin = 0;
+

The blank line isn't really necessary and it's a bool, so can you use 'false' rather than '0'?

Alternatively, it might be better to move:

		rsgl->sgl.need_unpin =
			iov_iter_extract_will_pin(&msg->msg_iter);

up instead.

David

author
#2 lore raw

Hi David,

David Howells <[email protected]> says:

Quoted 17 lines
Quoted 6 lines · Pavel Skripkin

Syzbot was able to trigger use of uninitialized memory in af_alg_free_resources.

Bug is caused by missing initialization of rsgl->sgl.need_unpin before adding to rsgl_list. Then in case of extract_iter_to_sg() failure, rsgl is left with uninitialized need_unpin which is read during clean up

Looks feasible :-).

crypto/af_alg.c int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags, · line 1245

Quoted 2 lines
crypto/af_alg.c +2 -0
+		rsgl->sgl.need_unpin = 0;
+

The blank line isn't really necessary and it's a bool, so can you use 'false' rather than '0'?

Quoted 7 lines

Alternatively, it might be better to move:

		rsgl->sgl.need_unpin =
			iov_iter_extract_will_pin(&msg->msg_iter);

up instead.

Thank you for review! I've just posted v2 :)

With regards,
Pavel Skripkin

author
#3 lore raw

PATCH v2 1/1 crypto: fix uninit-value in af_alg_free_resources

In reply to Pavel Skripkin

Syzbot was able to trigger use of uninitialized memory in af_alg_free_resources.

Bug is caused by missing initialization of rsgl->sgl.need_unpin before adding to rsgl_list. Then in case of extract_iter_to_sg() failure, rsgl is left with uninitialized need_unpin which is read during clean up

BUG: KMSAN: uninit-value in af_alg_free_sg crypto/af_alg.c:545 [inline]
BUG: KMSAN: uninit-value in af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
BUG: KMSAN: uninit-value in af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 af_alg_free_sg crypto/af_alg.c:545 [inline]
 af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
 af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 _skcipher_recvmsg crypto/algif_skcipher.c:144 [inline]

...

Uninit was created at:

 slab_post_alloc_hook+0x12f/0xb70 mm/slab.h:767
 slab_alloc_node mm/slub.c:3470 [inline]
 __kmem_cache_alloc_node+0x536/0x8d0 mm/slub.c:3509
 __do_kmalloc_node mm/slab_common.c:984 [inline]
 __kmalloc+0x121/0x3c0 mm/slab_common.c:998
 kmalloc include/linux/slab.h:586 [inline]
 sock_kmalloc+0x128/0x1c0 net/core/sock.c:2683
 af_alg_alloc_areq+0x41/0x2a0 crypto/af_alg.c:1188
 _skcipher_recvmsg crypto/algif_skcipher.c:71 [inline]
Fixes c1abe6f570af ("crypto: af_alg: Use extract_iter_to_sg() to create scatterlists")
Reported-and-tested-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=cba21d50095623218389
Signed-off-by Pavel Skripkin <[email protected]>

---

Changes since v1:
- Move sgl.need_unpin initialization upper instead of pre-initializing it with false as suggested by David

---
 crypto/af_alg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
crypto/af_alg.c +2 -2
index 06b15b9f661c..10efb56d8b48 100644
@@ -1241,6 +1241,8 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags,
 				return -ENOMEM;
 		}
 
+		rsgl->sgl.need_unpin =
+			iov_iter_extract_will_pin(&msg->msg_iter);
 		rsgl->sgl.sgt.sgl = rsgl->sgl.sgl;
 		rsgl->sgl.sgt.nents = 0;
 		rsgl->sgl.sgt.orig_nents = 0;

@@ -1255,8 +1257,6 @@ int af_alg_get_rsgl(struct sock *sk, struct msghdr *msg, int flags,
 		}
 
 		sg_mark_end(rsgl->sgl.sgt.sgl + rsgl->sgl.sgt.nents - 1);
-		rsgl->sgl.need_unpin =
-			iov_iter_extract_will_pin(&msg->msg_iter);
 
 		/* chain the new scatterlist with previous one */
 		if (areq->last_rsgl)
Signature · 1 line
2.40.1
maintainer
#4 lore raw
Quoted 33 lines · Pavel Skripkin

Syzbot was able to trigger use of uninitialized memory in af_alg_free_resources.

Bug is caused by missing initialization of rsgl->sgl.need_unpin before adding to rsgl_list. Then in case of extract_iter_to_sg() failure, rsgl is left with uninitialized need_unpin which is read during clean up

BUG: KMSAN: uninit-value in af_alg_free_sg crypto/af_alg.c:545 [inline]
BUG: KMSAN: uninit-value in af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
BUG: KMSAN: uninit-value in af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 af_alg_free_sg crypto/af_alg.c:545 [inline]
 af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
 af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 _skcipher_recvmsg crypto/algif_skcipher.c:144 [inline]

...

Uninit was created at:

 slab_post_alloc_hook+0x12f/0xb70 mm/slab.h:767
 slab_alloc_node mm/slub.c:3470 [inline]
 __kmem_cache_alloc_node+0x536/0x8d0 mm/slub.c:3509
 __do_kmalloc_node mm/slab_common.c:984 [inline]
 __kmalloc+0x121/0x3c0 mm/slab_common.c:998
 kmalloc include/linux/slab.h:586 [inline]
 sock_kmalloc+0x128/0x1c0 net/core/sock.c:2683
 af_alg_alloc_areq+0x41/0x2a0 crypto/af_alg.c:1188
 _skcipher_recvmsg crypto/algif_skcipher.c:71 [inline]
Fixes c1abe6f570af ("crypto: af_alg: Use extract_iter_to_sg() to create scatterlists")
Reported-and-tested-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=cba21d50095623218389
Signed-off-by Pavel Skripkin <[email protected]>

---

Quoted 7 lines

Changes since v1:
- Move sgl.need_unpin initialization upper instead of pre-initializing it with false as suggested by David

---
 crypto/af_alg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch applied. 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
Reviewed-by on v2 1/1
#5 lore raw
In reply to Pavel Skripkin
Quoted 27 lines · Pavel Skripkin

Syzbot was able to trigger use of uninitialized memory in af_alg_free_resources.

Bug is caused by missing initialization of rsgl->sgl.need_unpin before adding to rsgl_list. Then in case of extract_iter_to_sg() failure, rsgl is left with uninitialized need_unpin which is read during clean up

BUG: KMSAN: uninit-value in af_alg_free_sg crypto/af_alg.c:545 [inline]
BUG: KMSAN: uninit-value in af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
BUG: KMSAN: uninit-value in af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 af_alg_free_sg crypto/af_alg.c:545 [inline]
 af_alg_free_areq_sgls crypto/af_alg.c:778 [inline]
 af_alg_free_resources+0x3d1/0xf60 crypto/af_alg.c:1117
 _skcipher_recvmsg crypto/algif_skcipher.c:144 [inline]

...

Uninit was created at:

 slab_post_alloc_hook+0x12f/0xb70 mm/slab.h:767
 slab_alloc_node mm/slub.c:3470 [inline]
 __kmem_cache_alloc_node+0x536/0x8d0 mm/slub.c:3509
 __do_kmalloc_node mm/slab_common.c:984 [inline]
 __kmalloc+0x121/0x3c0 mm/slab_common.c:998
 kmalloc include/linux/slab.h:586 [inline]
 sock_kmalloc+0x128/0x1c0 net/core/sock.c:2683
 af_alg_alloc_areq+0x41/0x2a0 crypto/af_alg.c:1188
 _skcipher_recvmsg crypto/algif_skcipher.c:71 [inline]
Quoted 4 lines
Fixes c1abe6f570af ("crypto: af_alg: Use extract_iter_to_sg() to create scatterlists")
Reported-and-tested-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=cba21d50095623218389
Signed-off-by Pavel Skripkin <[email protected]>
Reviewed-by David Howells <[email protected]>