* [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume()
@ 2023-04-03 7:54 Ziyang Xuan
2023-04-03 15:01 ` Manivannan Sadhasivam
2023-04-08 8:35 ` Manivannan Sadhasivam
0 siblings, 2 replies; 6+ messages in thread
From: Ziyang Xuan @ 2023-04-03 7:54 UTC (permalink / raw)
To: mani, davem, edumazet, kuba, pabeni, andersson, luca,
linux-arm-msm, netdev
Syzbot reported a bug as following:
=====================================================
BUG: KMSAN: uninit-value in qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
qrtr_endpoint_post+0xf85/0x11b0 net/qrtr/af_qrtr.c:519
qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
call_write_iter include/linux/fs.h:2189 [inline]
aio_write+0x63a/0x950 fs/aio.c:1600
io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
__do_sys_io_submit fs/aio.c:2078 [inline]
__se_sys_io_submit+0x293/0x770 fs/aio.c:2048
__x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Uninit was created at:
slab_post_alloc_hook mm/slab.h:766 [inline]
slab_alloc_node mm/slub.c:3452 [inline]
__kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
__do_kmalloc_node mm/slab_common.c:967 [inline]
__kmalloc_node_track_caller+0x114/0x3b0 mm/slab_common.c:988
kmalloc_reserve net/core/skbuff.c:492 [inline]
__alloc_skb+0x3af/0x8f0 net/core/skbuff.c:565
__netdev_alloc_skb+0x120/0x7d0 net/core/skbuff.c:630
qrtr_endpoint_post+0xbd/0x11b0 net/qrtr/af_qrtr.c:446
qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
call_write_iter include/linux/fs.h:2189 [inline]
aio_write+0x63a/0x950 fs/aio.c:1600
io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
__do_sys_io_submit fs/aio.c:2078 [inline]
__se_sys_io_submit+0x293/0x770 fs/aio.c:2048
__x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
It is because that skb->len requires at least sizeof(struct qrtr_ctrl_pkt)
in qrtr_tx_resume(). And skb->len equals to size in qrtr_endpoint_post().
But size is less than sizeof(struct qrtr_ctrl_pkt) when qrtr_cb->type
equals to QRTR_TYPE_RESUME_TX in qrtr_endpoint_post() under the syzbot
scenario. This triggers the uninit variable access bug.
Add size check when qrtr_cb->type equals to QRTR_TYPE_RESUME_TX in
qrtr_endpoint_post() to fix the bug.
Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")
Reported-by: syzbot+4436c9630a45820fda76@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=c14607f0963d27d5a3d5f4c8639b500909e43540
Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
---
net/qrtr/af_qrtr.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 3a70255c8d02..631e81a8a368 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -498,6 +498,10 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
if (!size || len != ALIGN(size, 4) + hdrlen)
goto err;
+ if (cb->type == QRTR_TYPE_RESUME_TX &&
+ size < sizeof(struct qrtr_ctrl_pkt))
+ goto err;
+
if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
cb->type != QRTR_TYPE_RESUME_TX)
goto err;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume()
2023-04-03 7:54 [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume() Ziyang Xuan
@ 2023-04-03 15:01 ` Manivannan Sadhasivam
2023-04-04 0:38 ` Ziyang Xuan (William)
2023-04-08 8:35 ` Manivannan Sadhasivam
1 sibling, 1 reply; 6+ messages in thread
From: Manivannan Sadhasivam @ 2023-04-03 15:01 UTC (permalink / raw)
To: Ziyang Xuan
Cc: davem, edumazet, kuba, pabeni, andersson, luca, linux-arm-msm,
netdev
On Mon, Apr 03, 2023 at 03:54:17PM +0800, Ziyang Xuan wrote:
> Syzbot reported a bug as following:
>
> =====================================================
> BUG: KMSAN: uninit-value in qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
> qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
> qrtr_endpoint_post+0xf85/0x11b0 net/qrtr/af_qrtr.c:519
> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
> call_write_iter include/linux/fs.h:2189 [inline]
> aio_write+0x63a/0x950 fs/aio.c:1600
> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
> __do_sys_io_submit fs/aio.c:2078 [inline]
> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Uninit was created at:
> slab_post_alloc_hook mm/slab.h:766 [inline]
> slab_alloc_node mm/slub.c:3452 [inline]
> __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
> __do_kmalloc_node mm/slab_common.c:967 [inline]
> __kmalloc_node_track_caller+0x114/0x3b0 mm/slab_common.c:988
> kmalloc_reserve net/core/skbuff.c:492 [inline]
> __alloc_skb+0x3af/0x8f0 net/core/skbuff.c:565
> __netdev_alloc_skb+0x120/0x7d0 net/core/skbuff.c:630
> qrtr_endpoint_post+0xbd/0x11b0 net/qrtr/af_qrtr.c:446
> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
> call_write_iter include/linux/fs.h:2189 [inline]
> aio_write+0x63a/0x950 fs/aio.c:1600
> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
> __do_sys_io_submit fs/aio.c:2078 [inline]
> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> It is because that skb->len requires at least sizeof(struct qrtr_ctrl_pkt)
> in qrtr_tx_resume(). And skb->len equals to size in qrtr_endpoint_post().
> But size is less than sizeof(struct qrtr_ctrl_pkt) when qrtr_cb->type
> equals to QRTR_TYPE_RESUME_TX in qrtr_endpoint_post() under the syzbot
> scenario. This triggers the uninit variable access bug.
>
I'm not familiar with syzkaller. Can you please share the data that was fuzzed
by the bot?
- Mani
> Add size check when qrtr_cb->type equals to QRTR_TYPE_RESUME_TX in
> qrtr_endpoint_post() to fix the bug.
>
> Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")
> Reported-by: syzbot+4436c9630a45820fda76@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=c14607f0963d27d5a3d5f4c8639b500909e43540
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
> ---
> net/qrtr/af_qrtr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 3a70255c8d02..631e81a8a368 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -498,6 +498,10 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
> if (!size || len != ALIGN(size, 4) + hdrlen)
> goto err;
>
> + if (cb->type == QRTR_TYPE_RESUME_TX &&
> + size < sizeof(struct qrtr_ctrl_pkt))
> + goto err;
> +
> if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
> cb->type != QRTR_TYPE_RESUME_TX)
> goto err;
> --
> 2.25.1
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume()
2023-04-03 15:01 ` Manivannan Sadhasivam
@ 2023-04-04 0:38 ` Ziyang Xuan (William)
2023-04-06 8:34 ` Paolo Abeni
0 siblings, 1 reply; 6+ messages in thread
From: Ziyang Xuan (William) @ 2023-04-04 0:38 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: davem, edumazet, kuba, pabeni, andersson, luca, linux-arm-msm,
netdev
> On Mon, Apr 03, 2023 at 03:54:17PM +0800, Ziyang Xuan wrote:
>> Syzbot reported a bug as following:
>>
>> =====================================================
>> BUG: KMSAN: uninit-value in qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
>> qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
>> qrtr_endpoint_post+0xf85/0x11b0 net/qrtr/af_qrtr.c:519
>> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
>> call_write_iter include/linux/fs.h:2189 [inline]
>> aio_write+0x63a/0x950 fs/aio.c:1600
>> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
>> __do_sys_io_submit fs/aio.c:2078 [inline]
>> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
>> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Uninit was created at:
>> slab_post_alloc_hook mm/slab.h:766 [inline]
>> slab_alloc_node mm/slub.c:3452 [inline]
>> __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
>> __do_kmalloc_node mm/slab_common.c:967 [inline]
>> __kmalloc_node_track_caller+0x114/0x3b0 mm/slab_common.c:988
>> kmalloc_reserve net/core/skbuff.c:492 [inline]
>> __alloc_skb+0x3af/0x8f0 net/core/skbuff.c:565
>> __netdev_alloc_skb+0x120/0x7d0 net/core/skbuff.c:630
>> qrtr_endpoint_post+0xbd/0x11b0 net/qrtr/af_qrtr.c:446
>> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
>> call_write_iter include/linux/fs.h:2189 [inline]
>> aio_write+0x63a/0x950 fs/aio.c:1600
>> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
>> __do_sys_io_submit fs/aio.c:2078 [inline]
>> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
>> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> It is because that skb->len requires at least sizeof(struct qrtr_ctrl_pkt)
>> in qrtr_tx_resume(). And skb->len equals to size in qrtr_endpoint_post().
>> But size is less than sizeof(struct qrtr_ctrl_pkt) when qrtr_cb->type
>> equals to QRTR_TYPE_RESUME_TX in qrtr_endpoint_post() under the syzbot
>> scenario. This triggers the uninit variable access bug.
>>
>
> I'm not familiar with syzkaller. Can you please share the data that was fuzzed
> by the bot?
>
> - Mani
>
>> Add size check when qrtr_cb->type equals to QRTR_TYPE_RESUME_TX in
>> qrtr_endpoint_post() to fix the bug.
>>
>> Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")
>> Reported-by: syzbot+4436c9630a45820fda76@syzkaller.appspotmail.com
>> Link: https://syzkaller.appspot.com/bug?id=c14607f0963d27d5a3d5f4c8639b500909e43540
Hello Manivannan Sadhasivam
See the above link, it's syzkaller dashboard link, you can find a C reproducer that will help you.
William Xuan.
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>> ---
>> net/qrtr/af_qrtr.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
>> index 3a70255c8d02..631e81a8a368 100644
>> --- a/net/qrtr/af_qrtr.c
>> +++ b/net/qrtr/af_qrtr.c
>> @@ -498,6 +498,10 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>> if (!size || len != ALIGN(size, 4) + hdrlen)
>> goto err;
>>
>> + if (cb->type == QRTR_TYPE_RESUME_TX &&
>> + size < sizeof(struct qrtr_ctrl_pkt))
>> + goto err;
>> +
>> if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
>> cb->type != QRTR_TYPE_RESUME_TX)
>> goto err;
>> --
>> 2.25.1
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume()
2023-04-04 0:38 ` Ziyang Xuan (William)
@ 2023-04-06 8:34 ` Paolo Abeni
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2023-04-06 8:34 UTC (permalink / raw)
To: Ziyang Xuan (William), Manivannan Sadhasivam
Cc: davem, edumazet, kuba, andersson, luca, linux-arm-msm, netdev
Hi,
On Tue, 2023-04-04 at 08:38 +0800, Ziyang Xuan (William) wrote:
> > On Mon, Apr 03, 2023 at 03:54:17PM +0800, Ziyang Xuan wrote:
> > > Syzbot reported a bug as following:
> > >
> > > =====================================================
> > > BUG: KMSAN: uninit-value in qrtr_tx_resume+0x185/0x1f0
> > > net/qrtr/af_qrtr.c:230
> > > qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
> > > qrtr_endpoint_post+0xf85/0x11b0 net/qrtr/af_qrtr.c:519
> > > qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
> > > call_write_iter include/linux/fs.h:2189 [inline]
> > > aio_write+0x63a/0x950 fs/aio.c:1600
> > > io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
> > > __do_sys_io_submit fs/aio.c:2078 [inline]
> > > __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
> > > __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
> > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > > do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > >
> > > Uninit was created at:
> > > slab_post_alloc_hook mm/slab.h:766 [inline]
> > > slab_alloc_node mm/slub.c:3452 [inline]
> > > __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
> > > __do_kmalloc_node mm/slab_common.c:967 [inline]
> > > __kmalloc_node_track_caller+0x114/0x3b0 mm/slab_common.c:988
> > > kmalloc_reserve net/core/skbuff.c:492 [inline]
> > > __alloc_skb+0x3af/0x8f0 net/core/skbuff.c:565
> > > __netdev_alloc_skb+0x120/0x7d0 net/core/skbuff.c:630
> > > qrtr_endpoint_post+0xbd/0x11b0 net/qrtr/af_qrtr.c:446
> > > qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
> > > call_write_iter include/linux/fs.h:2189 [inline]
> > > aio_write+0x63a/0x950 fs/aio.c:1600
> > > io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
> > > __do_sys_io_submit fs/aio.c:2078 [inline]
> > > __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
> > > __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
> > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > > do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > >
> > > It is because that skb->len requires at least sizeof(struct
> > > qrtr_ctrl_pkt)
> > > in qrtr_tx_resume(). And skb->len equals to size in
> > > qrtr_endpoint_post().
> > > But size is less than sizeof(struct qrtr_ctrl_pkt) when qrtr_cb-
> > > >type
> > > equals to QRTR_TYPE_RESUME_TX in qrtr_endpoint_post() under the
> > > syzbot
> > > scenario. This triggers the uninit variable access bug.
> > >
> >
> > I'm not familiar with syzkaller. Can you please share the data that
> > was fuzzed
> > by the bot?
> >
> > - Mani
> >
> > > Add size check when qrtr_cb->type equals to QRTR_TYPE_RESUME_TX
> > > in
> > > qrtr_endpoint_post() to fix the bug.
> > >
> > > Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow
> > > control")
> > > Reported-by:
> > > syzbot+4436c9630a45820fda76@syzkaller.appspotmail.com
> > > Link:
> > > https://syzkaller.appspot.com/bug?id=c14607f0963d27d5a3d5f4c8639b500909e43540
>
> Hello Manivannan Sadhasivam
>
> See the above link, it's syzkaller dashboard link, you can find a C
> reproducer that will help you.
Hi Mani,
Are you satisfied with the information above? The patch LGTM and the
syzkaller report looks quite clear.
Thanks!
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume()
2023-04-03 7:54 [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume() Ziyang Xuan
2023-04-03 15:01 ` Manivannan Sadhasivam
@ 2023-04-08 8:35 ` Manivannan Sadhasivam
2023-04-10 0:58 ` Ziyang Xuan (William)
1 sibling, 1 reply; 6+ messages in thread
From: Manivannan Sadhasivam @ 2023-04-08 8:35 UTC (permalink / raw)
To: Ziyang Xuan
Cc: davem, edumazet, kuba, pabeni, andersson, luca, linux-arm-msm,
netdev
On Mon, Apr 03, 2023 at 03:54:17PM +0800, Ziyang Xuan wrote:
> Syzbot reported a bug as following:
>
> =====================================================
> BUG: KMSAN: uninit-value in qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
> qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
> qrtr_endpoint_post+0xf85/0x11b0 net/qrtr/af_qrtr.c:519
> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
> call_write_iter include/linux/fs.h:2189 [inline]
> aio_write+0x63a/0x950 fs/aio.c:1600
> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
> __do_sys_io_submit fs/aio.c:2078 [inline]
> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Uninit was created at:
> slab_post_alloc_hook mm/slab.h:766 [inline]
> slab_alloc_node mm/slub.c:3452 [inline]
> __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
> __do_kmalloc_node mm/slab_common.c:967 [inline]
> __kmalloc_node_track_caller+0x114/0x3b0 mm/slab_common.c:988
> kmalloc_reserve net/core/skbuff.c:492 [inline]
> __alloc_skb+0x3af/0x8f0 net/core/skbuff.c:565
> __netdev_alloc_skb+0x120/0x7d0 net/core/skbuff.c:630
> qrtr_endpoint_post+0xbd/0x11b0 net/qrtr/af_qrtr.c:446
> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
> call_write_iter include/linux/fs.h:2189 [inline]
> aio_write+0x63a/0x950 fs/aio.c:1600
> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
> __do_sys_io_submit fs/aio.c:2078 [inline]
> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> It is because that skb->len requires at least sizeof(struct qrtr_ctrl_pkt)
> in qrtr_tx_resume(). And skb->len equals to size in qrtr_endpoint_post().
> But size is less than sizeof(struct qrtr_ctrl_pkt) when qrtr_cb->type
> equals to QRTR_TYPE_RESUME_TX in qrtr_endpoint_post() under the syzbot
> scenario. This triggers the uninit variable access bug.
>
> Add size check when qrtr_cb->type equals to QRTR_TYPE_RESUME_TX in
> qrtr_endpoint_post() to fix the bug.
>
> Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")
> Reported-by: syzbot+4436c9630a45820fda76@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=c14607f0963d27d5a3d5f4c8639b500909e43540
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
Looks good to me. But I have a small suggestion below.
> ---
> net/qrtr/af_qrtr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 3a70255c8d02..631e81a8a368 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -498,6 +498,10 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
> if (!size || len != ALIGN(size, 4) + hdrlen)
> goto err;
>
> + if (cb->type == QRTR_TYPE_RESUME_TX &&
> + size < sizeof(struct qrtr_ctrl_pkt))
There is already a check for QRTR_TYPE_NEW_SERVER below. So you can combine both:
if ((cb->type == QRTR_TYPE_NEW_SERVER ||
cb->type == QRTR_TYPE_RESUME_TX)
&& size < sizeof(struct qrtr_ctrl_pkt))
goto err;
- Mani
> + goto err;
> +
> if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
> cb->type != QRTR_TYPE_RESUME_TX)
> goto err;
> --
> 2.25.1
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume()
2023-04-08 8:35 ` Manivannan Sadhasivam
@ 2023-04-10 0:58 ` Ziyang Xuan (William)
0 siblings, 0 replies; 6+ messages in thread
From: Ziyang Xuan (William) @ 2023-04-10 0:58 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: davem, edumazet, kuba, pabeni, andersson, luca, linux-arm-msm,
netdev
> On Mon, Apr 03, 2023 at 03:54:17PM +0800, Ziyang Xuan wrote:
>> Syzbot reported a bug as following:
>>
>> =====================================================
>> BUG: KMSAN: uninit-value in qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
>> qrtr_tx_resume+0x185/0x1f0 net/qrtr/af_qrtr.c:230
>> qrtr_endpoint_post+0xf85/0x11b0 net/qrtr/af_qrtr.c:519
>> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
>> call_write_iter include/linux/fs.h:2189 [inline]
>> aio_write+0x63a/0x950 fs/aio.c:1600
>> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
>> __do_sys_io_submit fs/aio.c:2078 [inline]
>> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
>> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Uninit was created at:
>> slab_post_alloc_hook mm/slab.h:766 [inline]
>> slab_alloc_node mm/slub.c:3452 [inline]
>> __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
>> __do_kmalloc_node mm/slab_common.c:967 [inline]
>> __kmalloc_node_track_caller+0x114/0x3b0 mm/slab_common.c:988
>> kmalloc_reserve net/core/skbuff.c:492 [inline]
>> __alloc_skb+0x3af/0x8f0 net/core/skbuff.c:565
>> __netdev_alloc_skb+0x120/0x7d0 net/core/skbuff.c:630
>> qrtr_endpoint_post+0xbd/0x11b0 net/qrtr/af_qrtr.c:446
>> qrtr_tun_write_iter+0x270/0x400 net/qrtr/tun.c:108
>> call_write_iter include/linux/fs.h:2189 [inline]
>> aio_write+0x63a/0x950 fs/aio.c:1600
>> io_submit_one+0x1d1c/0x3bf0 fs/aio.c:2019
>> __do_sys_io_submit fs/aio.c:2078 [inline]
>> __se_sys_io_submit+0x293/0x770 fs/aio.c:2048
>> __x64_sys_io_submit+0x92/0xd0 fs/aio.c:2048
>> do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> It is because that skb->len requires at least sizeof(struct qrtr_ctrl_pkt)
>> in qrtr_tx_resume(). And skb->len equals to size in qrtr_endpoint_post().
>> But size is less than sizeof(struct qrtr_ctrl_pkt) when qrtr_cb->type
>> equals to QRTR_TYPE_RESUME_TX in qrtr_endpoint_post() under the syzbot
>> scenario. This triggers the uninit variable access bug.
>>
>> Add size check when qrtr_cb->type equals to QRTR_TYPE_RESUME_TX in
>> qrtr_endpoint_post() to fix the bug.
>>
>> Fixes: 5fdeb0d372ab ("net: qrtr: Implement outgoing flow control")
>> Reported-by: syzbot+4436c9630a45820fda76@syzkaller.appspotmail.com
>> Link: https://syzkaller.appspot.com/bug?id=c14607f0963d27d5a3d5f4c8639b500909e43540
>> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>
>
> Looks good to me. But I have a small suggestion below.
>
>> ---
>> net/qrtr/af_qrtr.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
>> index 3a70255c8d02..631e81a8a368 100644
>> --- a/net/qrtr/af_qrtr.c
>> +++ b/net/qrtr/af_qrtr.c
>> @@ -498,6 +498,10 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>> if (!size || len != ALIGN(size, 4) + hdrlen)
>> goto err;
>>
>> + if (cb->type == QRTR_TYPE_RESUME_TX &&
>> + size < sizeof(struct qrtr_ctrl_pkt))
>
> There is already a check for QRTR_TYPE_NEW_SERVER below. So you can combine both:
>
> if ((cb->type == QRTR_TYPE_NEW_SERVER ||
> cb->type == QRTR_TYPE_RESUME_TX)
> && size < sizeof(struct qrtr_ctrl_pkt))
> goto err;
>
> - Mani
That's good. Thank you for your suggestion!
v2 will be sent.
William Xuan
>
>> + goto err;
>> +
>> if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
>> cb->type != QRTR_TYPE_RESUME_TX)
>> goto err;
>> --
>> 2.25.1
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-10 0:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-03 7:54 [PATCH net] net: qrtr: Fix an uninit variable access bug in qrtr_tx_resume() Ziyang Xuan
2023-04-03 15:01 ` Manivannan Sadhasivam
2023-04-04 0:38 ` Ziyang Xuan (William)
2023-04-06 8:34 ` Paolo Abeni
2023-04-08 8:35 ` Manivannan Sadhasivam
2023-04-10 0:58 ` Ziyang Xuan (William)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).