Received-SPF: pass (google.com: domain of linux-kernel+bounces-139836-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 11 Apr 2024 15:16:32 +0900
From: Ryosuke Yasuoka <ryasuoka@redhat.com>
To: Eric Dumazet <edumazet@google.com>
Cc: krzysztof.kozlowski@linaro.org, davem@davemloft.net, kuba@kernel.org,
	pabeni@redhat.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, syoshida@redhat.com
Subject: Re: [PATCH net] nfc: nci: Fix uninit-value in nci_rx_work
Message-ID: <ZheAQACKb82PkWkn@zeus>
References: <ZhJTu7qmOtTs9u2c@zeus>
 <CANn89iJrQevxPFLCj2P=U+XSisYD0jqrUQpa=zWMXTjj5+RriA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANn89iJrQevxPFLCj2P=U+XSisYD0jqrUQpa=zWMXTjj5+RriA@mail.gmail.com>

Thank you for your review, Eric.

On Mon, Apr 08, 2024 at 11:55:54AM +0200, Eric Dumazet wrote:
> On Sun, Apr 7, 2024 at 10:05 AM Ryosuke Yasuoka <ryasuoka@redhat.com> wrote:
> >
> > syzbot reported the following uninit-value access issue [1]
> >
> > nci_rx_work() parses received packet from ndev->rx_q. It should be
> > checked skb->len is non-zero to verify if it is valid before processing
> > the packet. If skb->len is zero but skb->data is not, such packet is
> > invalid and should be silently discarded.
> >
> > Fixes: d24b03535e5e ("nfc: nci: Fix uninit-value in nci_dev_up and nci_ntf_packet")
> > Reported-and-tested-by: syzbot+d7b4dc6cd50410152534@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=d7b4dc6cd50410152534 [1]
> > Signed-off-by: Ryosuke Yasuoka <ryasuoka@redhat.com>
> > ---
> >  net/nfc/nci/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> > index 0d26c8ec9993..b7a020484131 100644
> > --- a/net/nfc/nci/core.c
> > +++ b/net/nfc/nci/core.c
> > @@ -1516,7 +1516,7 @@ 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)) {
> > +               if (!skb->len || !nci_plen(skb->data)) {
> 
> #define nci_plen(hdr)           (__u8)((hdr)[2])
> 
> So your patch will not help if skb->len is 1 or 2.

Exactly. I've reviewed my patch and here's a draft. I'd like to hear
your and all member's opinion. Note this code currently redundant. Let
me explain why I've written it this way. I'll refine it before sending
the v2 patch.

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index b7a020484131..89bdb959080c 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1516,7 +1516,22 @@ 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 (!skb->len || !nci_plen(skb->data)) {
+		// FIXME: hardcoded
+		if (skb->len < 3) {
+			/* packet is too small */
+			kfree_skb(skb);
+			break;
+		}
+
+		if (!nci_plen(skb->data)) {
+			/* payload should not be zero */
+			kfree_skb(skb);
+			break;
+		}
+
+		// FIXME: hardcoded
+		if (skb->len < 3 + nci_plen(skb->data)) {
+			/* packet length is not match with actual packet size */
 			kfree_skb(skb);
 			break;
 		}

Before refering to message type in nci_mt(skb->data), we need to check a
couple of things below.

1. check if skb->len is larger than 3 Octets, which is header size of
NCI packet. Since the payload size is in 3rd octet of header, We should
check in advance.
2. check if nci packet payload is non zero as my previous fix
(03456156).
3. check if skb->len is match with actual NCI packet size

Could you check if these conditions are reasonable? If yes, then I need to
remove hardcorded "3", which represents the header size of NCI packet.
Now we can use NCI_CTRL_HDR_SIZE and NCI_DATA_HDR_SIZE instead of the
hardcorded value. However, we have no way to know if this NCI packet 
is data or ctrl packet before refering to message type in nci_mt(skb->data). 
In the current specification, since both of header sizes are the same, we can
check like this. 

-		if (skb->len < 3) {
+		if (skb->len < NCI_CTRL_HDR_SIZE ||
+		    skb->len < NCI_DATA_HDR_SIZE) {

But it has potential bug that packet will drop unexpectedly if either 
ctrl or data header size become larger.

Do you have any good idea to implement this condition?

Regards,
Ryosuke