Re: [PATCH net] nfc: nci: Fix uninit-value in nci_dev_up

[Paolo Abeni]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Closes: https://syzkaller.appspot.com/bug?extid=7ea9413ea6749baf5574 [1]
> > > Signed-off-by: Ryosuke Yasuoka <ryasuoka@xxxxxxxxxx>
> > > ---
> > >  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