[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <498dc99d25965877f7d15592dcbb340f97d803b4.camel@redhat.com>
Date: Thu, 14 Mar 2024 12:58:14 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Ryosuke Yasuoka <ryasuoka@...hat.com>, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org, syoshida@...hat.com,
syzbot+7ea9413ea6749baf5574@...kaller.appspotmail.com
Subject: Re: [PATCH net] nfc: nci: Fix uninit-value in nci_dev_up
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@...kaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
> > > Signed-off-by: Ryosuke Yasuoka <ryasuoka@...hat.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
Powered by blists - more mailing lists