From: Simon Horman <horms@kernel.org>
To: Ryosuke Yasuoka <ryasuoka@redhat.com>
Cc: krzk@kernel.org, davem@davemloft.net, edumazet@google.com,
kuba@kernel.org, pabeni@redhat.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, syoshida@redhat.com,
syzbot+d7b4dc6cd50410152534@syzkaller.appspotmail.com
Subject: Re: [PATCH net v2] nfc: nci: Fix uninit-value in nci_rx_work
Date: Mon, 29 Apr 2024 17:41:18 +0100 [thread overview]
Message-ID: <20240429164118.GB516117@kernel.org> (raw)
In-Reply-To: <Zi-vGH1ROjiv1yJ2@zeus>
On Mon, Apr 29, 2024 at 11:30:48PM +0900, Ryosuke Yasuoka wrote:
> On Sun, Apr 28, 2024 at 02:45:25PM +0100, Simon Horman wrote:
> > On Sat, Apr 27, 2024 at 07:35:54PM +0900, Ryosuke Yasuoka wrote:
...
> Thank you for your comment, Simon.
>
> Yes, if it handles packets correctly in nci_{rsp,ntf,rx_data}_packet(),
> it should not reach invalid_pkt_free and it should continue to work in
> the for statement. Sorry, it is my mistake and need to fix it.
>
> BTW, in the current implementation, if the payload is zero, it will free
> the skb and exit the for statement. I wonder it is intended.
>
> > > - if (!nci_plen(skb->data)) {
> > > - kfree_skb(skb);
> > > - break;
> > > - }
>
> When the packet is invalid, it should be discarded but it should not
> exit the for statement by break. Instead, the skb should just free and
> it should handle the subsequent packet by continue. If yes, then it
> may be like below,
>
> for (; (skb = skb_dequeue(&ndev->rx_q)); kcov_remote_stop()) {
> kcov_remote_start_common(skb_get_kcov_handle(skb));
>
> /* Send copy to sniffer */
> nfc_send_to_raw_sock(ndev->nfc_dev, skb,
> RAW_PAYLOAD_NCI, NFC_DIRECTION_RX);
>
> if (!skb->len)
> goto invalid_pkt_free;
>
> /* Process frame */
> switch (nci_mt(skb->data)) {
> case NCI_MT_RSP_PKT:
> if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> goto invalid_pkt_free;
> nci_rsp_packet(ndev, skb);
> continue; <<<---
>
> case NCI_MT_NTF_PKT:
> if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> goto invalid_pkt_free;
> nci_ntf_packet(ndev, skb);
> continue; <<<---
>
> case NCI_MT_DATA_PKT:
> if (!nci_valid_size(skb, NCI_DATA_HDR_SIZE))
> goto invalid_pkt_free;
> nci_rx_data_packet(ndev, skb);
> continue; <<<---
>
> default:
> pr_err("unknown MT 0x%x\n", nci_mt(skb->data));
> goto invalid_pkt_free;
> }
> invalid_pkt_free:
> kfree_skb(skb);
> }
>
> Could I hear your opinion?
Hi Yasuoka-san,
Thanks for pointing this out.
I agree that it is not good to 'break' after kfree_skb() for two reasons:
1. As you mention, the loop should keep going and process other skbs
2. kcov_remote_stop() needs to be called for each skb
I might have used a 'continue' above the invalid_pkt_free label.
But I think your suggestion - using 'continue' inside the switch statement
- is also correct, and seems fine to me.
Please post a v3 when you are ready.
--
pw-bot: changes-requested
prev parent reply other threads:[~2024-04-29 16:41 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-27 10:35 [PATCH net v2] nfc: nci: Fix uninit-value in nci_rx_work Ryosuke Yasuoka
2024-04-28 13:45 ` Simon Horman
2024-04-29 14:30 ` Ryosuke Yasuoka
2024-04-29 16:41 ` Simon Horman [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240429164118.GB516117@kernel.org \
--to=horms@kernel.org \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=krzk@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=ryasuoka@redhat.com \
--cc=syoshida@redhat.com \
--cc=syzbot+d7b4dc6cd50410152534@syzkaller.appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.