Messages in this thread | | | Subject | Re: [PATCH net] nfc: nci: Fix uninit-value in nci_dev_up | From | Paolo Abeni <> | Date | Thu, 14 Mar 2024 12:58:14 +0100 |
| |
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: syzbot+7ea9413ea6749baf5574@syzkaller.appspotmail.com > > > Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1] > > > Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com> > > > --- > > > 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
| |