diff mbox series

crypto: fix uninit-value in af_alg_free_resources

Message ID 20230813122344.14142-1-paskripkin@gmail.com
State New
Headers
Series crypto: fix uninit-value in af_alg_free_resources |

Commit Message

Pavel Skripkin Aug. 13, 2023, 12:23 p.m. UTC 
  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: syzbot+cba21d50095623218389@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=cba21d50095623218389
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 crypto/af_alg.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

David Howells Aug. 14, 2023, 8:26 a.m. UTC | #1 
Pavel Skripkin <paskripkin@gmail.com> wrote:

> 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 :-).

> +		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
Pavel Skripkin Aug. 14, 2023, 6:04 p.m. UTC | #2 
Hi David,

David Howells <dhowells@redhat.com> says:
> Pavel Skripkin <paskripkin@gmail.com> wrote:
> 
>> 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 :-).
> 
>> +		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.
> 

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



With regards,
Pavel Skripkin
diff mbox series

Patch

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index 06b15b9f661c..4e4f2d837481 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -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;