linux-kernel - Re: [PATCH net] nfc: nci: Fix uninit-value in nci_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<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