syzbot reported the following unitit-value access issue [1]:
nci_ntf_packet() calls each ntf operation parsed from skb->data. When
the payload length is zero, each operation handler reads uninitialized
payload and KMSAN detects this issue. Such notification packet should be
silently discarded since it is unexpected for any notification handlers.
This patch resolved this issue by checking payload size before calling
each notification handler codes.
BUG: KMSAN: uninit-value in nci_init_req net/nfc/nci/core.c:177 [inline]
BUG: KMSAN: uninit-value in __nci_request net/nfc/nci/core.c:108 [inline]
BUG: KMSAN: uninit-value in nci_open_device net/nfc/nci/core.c:521 [inline]
BUG: KMSAN: uninit-value in nci_dev_up+0xfec/0x1b10 net/nfc/nci/core.c:632
nci_init_req net/nfc/nci/core.c:177 [inline]
__nci_request net/nfc/nci/core.c:108 [inline]
nci_open_device net/nfc/nci/core.c:521 [inline]
nci_dev_up+0xfec/0x1b10 net/nfc/nci/core.c:632
nfc_dev_up+0x26e/0x440 net/nfc/core.c:118
nfc_genl_dev_up+0xfe/0x1d0 net/nfc/netlink.c:770
genl_family_rcv_msg_doit net/netlink/genetlink.c:972 [inline]
genl_family_rcv_msg net/netlink/genetlink.c:1052 [inline]
genl_rcv_msg+0x11ec/0x1290 net/netlink/genetlink.c:1067
netlink_rcv_skb+0x371/0x650 net/netlink/af_netlink.c:2545
genl_rcv+0x40/0x60 net/netlink/genetlink.c:1076
netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
netlink_unicast+0xf47/0x1250 net/netlink/af_netlink.c:1368
netlink_sendmsg+0x1238/0x13d0 net/netlink/af_netlink.c:1910
sock_sendmsg_nosec net/socket.c:730 [inline]
__sock_sendmsg net/socket.c:745 [inline]
____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
___sys_sendmsg+0x28d/0x3c0 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+0x307/0x490 net/socket.c:2674
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0x6d/0x140 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b
Uninit was stored to memory at:
nci_core_reset_ntf_packet net/nfc/nci/ntf.c:36 [inline]
nci_ntf_packet+0x19dc/0x39c0 net/nfc/nci/ntf.c:782
nci_rx_work+0x213/0x500 net/nfc/nci/core.c:1522
process_one_work kernel/workqueue.c:2633 [inline]
process_scheduled_works+0x104e/0x1e70 kernel/workqueue.c:2706
worker_thread+0xf45/0x1490 kernel/workqueue.c:2787
kthread+0x3ed/0x540 kernel/kthread.c:388
ret_from_fork+0x66/0x80 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:242
Uninit was created at:
slab_post_alloc_hook+0x129/0xa70 mm/slab.h:768
slab_alloc_node mm/slub.c:3478 [inline]
kmem_cache_alloc_node+0x5e9/0xb10 mm/slub.c:3523
kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:560
__alloc_skb+0x318/0x740 net/core/skbuff.c:651
alloc_skb include/linux/skbuff.h:1286 [inline]
virtual_ncidev_write+0x6d/0x280 drivers/nfc/virtual_ncidev.c:120
vfs_write+0x48b/0x1200 fs/read_write.c:588
ksys_write+0x20f/0x4c0 fs/read_write.c:643
__do_sys_write fs/read_write.c:655 [inline]
__se_sys_write fs/read_write.c:652 [inline]
__x64_sys_write+0x93/0xd0 fs/read_write.c:652
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0x6d/0x140 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b
CPU: 1 PID: 5012 Comm: syz-executor935 Not tainted 6.7.0-syzkaller-00562-g9f8413c4a66f #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
Reported-and-tested-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
Signed-off-by: Ryosuke Yasuoka <[email protected]>
---
net/nfc/nci/ntf.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
index 994a0a1efb58..56624387e253 100644
--- a/net/nfc/nci/ntf.c
+++ b/net/nfc/nci/ntf.c
@@ -765,6 +765,9 @@ void nci_ntf_packet(struct nci_dev *ndev, struct sk_buff *skb)
nci_opcode_oid(ntf_opcode),
nci_plen(skb->data));
+ if (!nci_plen(skb->data))
+ goto end;
+
/* strip the nci control header */
skb_pull(skb, NCI_CTRL_HDR_SIZE);
--
2.44.0
On 12/03/2024 15:56, Ryosuke Yasuoka wrote:
> syzbot reported the following unitit-value access issue [1]:
If there is going to be any new version then: typo, uninit-value
>
> nci_ntf_packet() calls each ntf operation parsed from skb->data. When
> the payload length is zero, each operation handler reads uninitialized
> payload and KMSAN detects this issue. Such notification packet should be
> silently discarded since it is unexpected for any notification handlers.
>
> This patch resolved this issue by checking payload size before calling
> each notification handler codes.
>
> BUG: KMSAN: uninit-value in nci_init_req net/nfc/nci/core.c:177 [inline]
> BUG: KMSAN: uninit-value in __nci_request net/nfc/nci/core.c:108 [inline]
> BUG: KMSAN: uninit-value in nci_open_device net/nfc/nci/core.c:521 [inline]
> BUG: KMSAN: uninit-value in nci_dev_up+0xfec/0x1b10 net/nfc/nci/core.c:632
> nci_init_req net/nfc/nci/core.c:177 [inline]
> __nci_request net/nfc/nci/core.c:108 [inline]
> nci_open_device net/nfc/nci/core.c:521 [inline]
> nci_dev_up+0xfec/0x1b10 net/nfc/nci/core.c:632
> nfc_dev_up+0x26e/0x440 net/nfc/core.c:118
> nfc_genl_dev_up+0xfe/0x1d0 net/nfc/netlink.c:770
> genl_family_rcv_msg_doit net/netlink/genetlink.c:972 [inline]
> genl_family_rcv_msg net/netlink/genetlink.c:1052 [inline]
> genl_rcv_msg+0x11ec/0x1290 net/netlink/genetlink.c:1067
> netlink_rcv_skb+0x371/0x650 net/netlink/af_netlink.c:2545
> genl_rcv+0x40/0x60 net/netlink/genetlink.c:1076
> netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
> netlink_unicast+0xf47/0x1250 net/netlink/af_netlink.c:1368
> netlink_sendmsg+0x1238/0x13d0 net/netlink/af_netlink.c:1910
> sock_sendmsg_nosec net/socket.c:730 [inline]
> __sock_sendmsg net/socket.c:745 [inline]
> ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
> ___sys_sendmsg+0x28d/0x3c0 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+0x307/0x490 net/socket.c:2674
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x6d/0x140 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> Uninit was stored to memory at:
> nci_core_reset_ntf_packet net/nfc/nci/ntf.c:36 [inline]
> nci_ntf_packet+0x19dc/0x39c0 net/nfc/nci/ntf.c:782
> nci_rx_work+0x213/0x500 net/nfc/nci/core.c:1522
> process_one_work kernel/workqueue.c:2633 [inline]
> process_scheduled_works+0x104e/0x1e70 kernel/workqueue.c:2706
> worker_thread+0xf45/0x1490 kernel/workqueue.c:2787
> kthread+0x3ed/0x540 kernel/kthread.c:388
> ret_from_fork+0x66/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:242
>
> Uninit was created at:
> slab_post_alloc_hook+0x129/0xa70 mm/slab.h:768
> slab_alloc_node mm/slub.c:3478 [inline]
> kmem_cache_alloc_node+0x5e9/0xb10 mm/slub.c:3523
> kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:560
> __alloc_skb+0x318/0x740 net/core/skbuff.c:651
> alloc_skb include/linux/skbuff.h:1286 [inline]
> virtual_ncidev_write+0x6d/0x280 drivers/nfc/virtual_ncidev.c:120
> vfs_write+0x48b/0x1200 fs/read_write.c:588
> ksys_write+0x20f/0x4c0 fs/read_write.c:643
> __do_sys_write fs/read_write.c:655 [inline]
> __se_sys_write fs/read_write.c:652 [inline]
> __x64_sys_write+0x93/0xd0 fs/read_write.c:652
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x6d/0x140 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> CPU: 1 PID: 5012 Comm: syz-executor935 Not tainted 6.7.0-syzkaller-00562-g9f8413c4a66f #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
These two lines are not really relevant, it's a virtual platform, so
whether this is Google or Amazon it does not matter, and your log paste
is already quite long. If there is going to be any resend, I propose to
drop.
>
> Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
> Reported-and-tested-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
> Signed-off-by: Ryosuke Yasuoka <[email protected]>
> ---
> net/nfc/nci/ntf.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> index 994a0a1efb58..56624387e253 100644
> --- a/net/nfc/nci/ntf.c
> +++ b/net/nfc/nci/ntf.c
> @@ -765,6 +765,9 @@ void nci_ntf_packet(struct nci_dev *ndev, struct sk_buff *skb)
> nci_opcode_oid(ntf_opcode),
> nci_plen(skb->data));
>
> + if (!nci_plen(skb->data))
> + goto end;
Looks reasonable, however wouldn't there be the same issue in
nci_rsp_packet() and other cases from nci_rx_work()? I wonder why only
NTF packets could be constructed without payload.
Best regards,
Krzysztof
On Wed, Mar 13, 2024 at 10:01:27AM +0100, Krzysztof Kozlowski wrote:
> On 12/03/2024 15:56, Ryosuke Yasuoka wrote:
> > syzbot reported the following unitit-value access issue [1]:
>
> If there is going to be any new version then: typo, uninit-value
Thank you for pointing out. I'll fix it.
> >
> > nci_ntf_packet() calls each ntf operation parsed from skb->data. When
> > the payload length is zero, each operation handler reads uninitialized
> > payload and KMSAN detects this issue. Such notification packet should be
> > silently discarded since it is unexpected for any notification handlers.
> >
> > This patch resolved this issue by checking payload size before calling
> > each notification handler codes.
> >
> > BUG: KMSAN: uninit-value in nci_init_req net/nfc/nci/core.c:177 [inline]
> > BUG: KMSAN: uninit-value in __nci_request net/nfc/nci/core.c:108 [inline]
> > BUG: KMSAN: uninit-value in nci_open_device net/nfc/nci/core.c:521 [inline]
> > BUG: KMSAN: uninit-value in nci_dev_up+0xfec/0x1b10 net/nfc/nci/core.c:632
> > nci_init_req net/nfc/nci/core.c:177 [inline]
> > __nci_request net/nfc/nci/core.c:108 [inline]
> > nci_open_device net/nfc/nci/core.c:521 [inline]
> > nci_dev_up+0xfec/0x1b10 net/nfc/nci/core.c:632
> > nfc_dev_up+0x26e/0x440 net/nfc/core.c:118
> > nfc_genl_dev_up+0xfe/0x1d0 net/nfc/netlink.c:770
> > genl_family_rcv_msg_doit net/netlink/genetlink.c:972 [inline]
> > genl_family_rcv_msg net/netlink/genetlink.c:1052 [inline]
> > genl_rcv_msg+0x11ec/0x1290 net/netlink/genetlink.c:1067
> > netlink_rcv_skb+0x371/0x650 net/netlink/af_netlink.c:2545
> > genl_rcv+0x40/0x60 net/netlink/genetlink.c:1076
> > netlink_unicast_kernel net/netlink/af_netlink.c:1342 [inline]
> > netlink_unicast+0xf47/0x1250 net/netlink/af_netlink.c:1368
> > netlink_sendmsg+0x1238/0x13d0 net/netlink/af_netlink.c:1910
> > sock_sendmsg_nosec net/socket.c:730 [inline]
> > __sock_sendmsg net/socket.c:745 [inline]
> > ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
> > ___sys_sendmsg+0x28d/0x3c0 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+0x307/0x490 net/socket.c:2674
> > do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > do_syscall_64+0x6d/0x140 arch/x86/entry/common.c:83
> > entry_SYSCALL_64_after_hwframe+0x63/0x6b
> >
> > Uninit was stored to memory at:
> > nci_core_reset_ntf_packet net/nfc/nci/ntf.c:36 [inline]
> > nci_ntf_packet+0x19dc/0x39c0 net/nfc/nci/ntf.c:782
> > nci_rx_work+0x213/0x500 net/nfc/nci/core.c:1522
> > process_one_work kernel/workqueue.c:2633 [inline]
> > process_scheduled_works+0x104e/0x1e70 kernel/workqueue.c:2706
> > worker_thread+0xf45/0x1490 kernel/workqueue.c:2787
> > kthread+0x3ed/0x540 kernel/kthread.c:388
> > ret_from_fork+0x66/0x80 arch/x86/kernel/process.c:147
> > ret_from_fork_asm+0x11/0x20 arch/x86/entry/entry_64.S:242
> >
> > Uninit was created at:
> > slab_post_alloc_hook+0x129/0xa70 mm/slab.h:768
> > slab_alloc_node mm/slub.c:3478 [inline]
> > kmem_cache_alloc_node+0x5e9/0xb10 mm/slub.c:3523
> > kmalloc_reserve+0x13d/0x4a0 net/core/skbuff.c:560
> > __alloc_skb+0x318/0x740 net/core/skbuff.c:651
> > alloc_skb include/linux/skbuff.h:1286 [inline]
> > virtual_ncidev_write+0x6d/0x280 drivers/nfc/virtual_ncidev.c:120
> > vfs_write+0x48b/0x1200 fs/read_write.c:588
> > ksys_write+0x20f/0x4c0 fs/read_write.c:643
> > __do_sys_write fs/read_write.c:655 [inline]
> > __se_sys_write fs/read_write.c:652 [inline]
> > __x64_sys_write+0x93/0xd0 fs/read_write.c:652
> > do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> > do_syscall_64+0x6d/0x140 arch/x86/entry/common.c:83
> > entry_SYSCALL_64_after_hwframe+0x63/0x6b
> >
> > CPU: 1 PID: 5012 Comm: syz-executor935 Not tainted 6.7.0-syzkaller-00562-g9f8413c4a66f #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
>
> These two lines are not really relevant, it's a virtual platform, so
> whether this is Google or Amazon it does not matter, and your log paste
> is already quite long. If there is going to be any resend, I propose to
> drop.
OK. Do you mean all these log messages that syzbot reported should be
dropped or I should leave only relavant messages?
> >
> > Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
> > Reported-and-tested-by: [email protected]
> > Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
> > Signed-off-by: Ryosuke Yasuoka <[email protected]>
> > ---
> > net/nfc/nci/ntf.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> > index 994a0a1efb58..56624387e253 100644
> > --- a/net/nfc/nci/ntf.c
> > +++ b/net/nfc/nci/ntf.c
> > @@ -765,6 +765,9 @@ void nci_ntf_packet(struct nci_dev *ndev, struct sk_buff *skb)
> > nci_opcode_oid(ntf_opcode),
> > nci_plen(skb->data));
> >
> > + if (!nci_plen(skb->data))
> > + goto end;
>
> Looks reasonable, however wouldn't there be the same issue in
> nci_rsp_packet() and other cases from nci_rx_work()? I wonder why only
> NTF packets could be constructed without payload.
Yes, I can reproduced very similar bug reported by syzbot [2] in my lab.
When the MT is NCI_MT_RSP_PKT (0x2), KMSAN detects the following bug in
nci_rsp_packet().
[How to reproduce]
(gdb) b net/nfc/nci/core.c:1516
Breakpoint 2 at 0xffffffff8eba68bc: file net/nfc/nci/core.c, line 1516.
(gdb) c <<<--- Run the reproducer provided in [1]
..
1516 switch (nci_mt(skb->data)) {
(gdb) p skb->data
$2 = (unsigned char *) 0xffff88802e75d400 "`"
(gdb) p/x *(unsigned char *)0xffff88802e75d400
$4 = 0x60
(gdb) p/x *(unsigned char *)0xffff88802e75d400 = 0x50
$5 = 0x50
(gdb) c
..
[ 370.186593][ T8135] =====================================================
[ 370.188107][ T8135] BUG: KMSAN: uninit-value in nci_dev_up+0xfb4/0x1590
[ 370.189016][ T8135] nci_dev_up+0xfb4/0x1590
[ 370.189642][ T8135] nfc_dev_up+0x281/0x520
[ 370.190312][ T8135] nfc_genl_dev_up+0xf1/0x1c0
[ 370.190990][ T8135] genl_rcv_msg+0x1220/0x12c0
[ 370.192189][ T8135] netlink_rcv_skb+0x542/0x670
[ 370.192865][ T8135] genl_rcv+0x41/0x60
[ 370.193426][ T8135] netlink_unicast+0xf43/0x1220
[ 370.194118][ T8135] netlink_sendmsg+0x1242/0x1420
[ 370.194801][ T8135] ____sys_sendmsg+0x98b/0xd10
[ 370.195509][ T8135] ___sys_sendmsg+0x271/0x3b0
[ 370.196163][ T8135] __x64_sys_sendmsg+0x2ee/0x4b0
[ 370.196849][ T8135] do_syscall_64+0x6d/0x140
[ 370.197511][ T8135] entry_SYSCALL_64_after_hwframe+0x63/0x6b
[ 370.198362][ T8135]
[ 370.198681][ T8135] Uninit was stored to memory at:
[ 370.199392][ T8135] nci_dev_up+0xfad/0x1590
[ 370.200011][ T8135] nfc_dev_up+0x281/0x520
[ 370.200608][ T8135] nfc_genl_dev_up+0xf1/0x1c0
[ 370.201669][ T8135] genl_rcv_msg+0x1220/0x12c0
[ 370.202332][ T8135] netlink_rcv_skb+0x542/0x670
[ 370.203012][ T8135] genl_rcv+0x41/0x60
[ 370.203563][ T8135] netlink_unicast+0xf43/0x1220
[ 370.204250][ T8135] netlink_sendmsg+0x1242/0x1420
[ 370.204914][ T8135] ____sys_sendmsg+0x98b/0xd10
[ 370.205525][ T8135] ___sys_sendmsg+0x271/0x3b0
[ 370.206041][ T8135] __x64_sys_sendmsg+0x2ee/0x4b0
[ 370.206552][ T8135] do_syscall_64+0x6d/0x140
[ 370.207106][ T8135] entry_SYSCALL_64_after_hwframe+0x63/0x6b
[ 370.207841][ T8135]
[ 370.208160][ T8135] Uninit was stored to memory at:
[ 370.208749][ T8135] nci_rsp_packet+0x1daf/0x3b30
[ 370.209295][ T8135] nci_rx_work+0x1e8/0x530
[ 370.209746][ T8135] process_scheduled_works+0x1174/0x1e20
[ 370.210449][ T8135] worker_thread+0xea3/0x1490
[ 370.210989][ T8135] kthread+0x3d1/0x530
[ 370.211932][ T8135] ret_from_fork+0x66/0x80
[ 370.212580][ T8135] ret_from_fork_asm+0x11/0x20
345 void nci_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb)
346 {
347 __u16 rsp_opcode = nci_opcode(skb->data);
..
370 switch (rsp_opcode) {
371 case NCI_OP_CORE_RESET_RSP:
372 nci_core_reset_rsp_packet(ndev, skb);
28 static void nci_core_reset_rsp_packet(struct nci_dev *ndev,
29 const struct sk_buff *skb)
30 {
31 const struct nci_core_reset_rsp *rsp = (void *)skb->data;
...
36 if (skb->len != 1) {
37 if (rsp->status == NCI_STATUS_OK) {
38 ndev->nci_ver = rsp->nci_ver; <<<---
So it means we should check the payload length in not nci_ntf_packet()
but in nci_rx_work(). Like this. (Note that it has not tested.)
diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 6c9592d05120..f9880d6ad2b2 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1512,6 +1512,11 @@ static void nci_rx_work(struct work_struct *work)
nfc_send_to_raw_sock(ndev->nfc_dev, skb,
RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
+ if (!nci_plen(skb->data)) {
+ skb(free);
+ break;
+ }
+
/* Process frame */
switch (nci_mt(skb->data)) {
case NCI_MT_RSP_PKT:
Let me know if you have any idea.
https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
https://syzkaller.appspot.com/bug?extid=685805de744584f4d24b [2]
Best Regards,
Ryosuke
On Thu, 2024-03-14 at 18:59 +0900, Ryosuke Yasuoka wrote:
> On Wed, Mar 13, 2024 at 10:01:27AM +0100, Krzysztof Kozlowski wrote:
> > On 12/03/2024 15:56, Ryosuke Yasuoka wrote:
> >
> > > CPU: 1 PID: 5012 Comm: syz-executor935 Not tainted 6.7.0-syzkaller-00562-g9f8413c4a66f #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
> >
> > These two lines are not really relevant, it's a virtual platform, so
> > whether this is Google or Amazon it does not matter, and your log paste
> > is already quite long. If there is going to be any resend, I propose to
> > drop.
>
> OK. Do you mean all these log messages that syzbot reported should be
> dropped or I should leave only relavant messages?
It's not a big deal either way, but there is a quite established
practice of including the whole splat.
> > > Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
> > > Reported-and-tested-by: [email protected]
> > > Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
> > > Signed-off-by: Ryosuke Yasuoka <[email protected]>
> > > ---
> > > net/nfc/nci/ntf.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/net/nfc/nci/ntf.c b/net/nfc/nci/ntf.c
> > > index 994a0a1efb58..56624387e253 100644
> > > --- a/net/nfc/nci/ntf.c
> > > +++ b/net/nfc/nci/ntf.c
> > > @@ -765,6 +765,9 @@ void nci_ntf_packet(struct nci_dev *ndev, struct sk_buff *skb)
> > > nci_opcode_oid(ntf_opcode),
> > > nci_plen(skb->data));
> > >
> > > + if (!nci_plen(skb->data))
> > > + goto end;
> >
> > Looks reasonable, however wouldn't there be the same issue in
> > nci_rsp_packet() and other cases from nci_rx_work()? I wonder why only
> > NTF packets could be constructed without payload.
>
> Yes, I can reproduced very similar bug reported by syzbot [2] in my lab.
> When the MT is NCI_MT_RSP_PKT (0x2), KMSAN detects the following bug in
> nci_rsp_packet().
[...]
> So it means we should check the payload length in not >
nci_ntf_packet()
> but in nci_rx_work(). Like this. (Note that it has not tested.)
>
> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 6c9592d05120..f9880d6ad2b2 100644
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -1512,6 +1512,11 @@ static void nci_rx_work(struct work_struct *work)
> nfc_send_to_raw_sock(ndev->nfc_dev, skb,
> RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
>
> + if (!nci_plen(skb->data)) {
> + skb(free);
> + break;
> + }
> +
> /* Process frame */
> switch (nci_mt(skb->data)) {
> case NCI_MT_RSP_PKT:
>
> Let me know if you have any idea.
>
> https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
> https://syzkaller.appspot.com/bug?extid=685805de744584f4d24b [2]
I think addressing the issue early in the code path would be better -
unless there is some functional issue with that I can't foresee.
Thanks,
Paolo
On 14/03/2024 12:58, Paolo Abeni wrote:
> On Thu, 2024-03-14 at 18:59 +0900, Ryosuke Yasuoka wrote:
>> On Wed, Mar 13, 2024 at 10:01:27AM +0100, Krzysztof Kozlowski wrote:
>>> On 12/03/2024 15:56, Ryosuke Yasuoka wrote:
>>>
>>>> CPU: 1 PID: 5012 Comm: syz-executor935 Not tainted 6.7.0-syzkaller-00562-g9f8413c4a66f #0
>>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
>>>
>>> These two lines are not really relevant, it's a virtual platform, so
>>> whether this is Google or Amazon it does not matter, and your log paste
>>> is already quite long. If there is going to be any resend, I propose to
>>> drop.
>>
>> OK. Do you mean all these log messages that syzbot reported should be
>> dropped or I should leave only relavant messages?
No, I proposed to drop "these two lines". Please look at where people
put their comments and what do they trim from the response.
>
> It's not a big deal either way, but there is a quite established
> practice of including the whole splat.
Yeah, splat was fine in general.
Best regards,
Krzysztof