[PATCH net v4] nfc: nci: Fix uninit-value in nci_rx_work

[PATCH net v4] nfc: nci: Fix uninit-value in nci_rx_work

[Ryosuke Yasuoka]

syzbot reported the following uninit-value access issue [1]

nci_rx_work() parses received packet from ndev->rx_q. It should be
validated header size, payload size and total packet size before
processing the packet. If an invalid packet is detected, it 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>
---
v4
- v3 patch uses goto statement and it makes codes complicated. So this
  patch simply calls kfree_skb inside loop and remove goto statement.
- [2] inserted kcov_remote_stop() to fix kcov check. However, as we
  discuss about my v3 patch [3], it should not exit the for statement
  and should continue processing subsequent packets. This patch removes
  them and simply insert continue statement.

[2] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=19e35f24750d

v3
https://lore.kernel.org/netdev/20240502082323.250739-1-ryasuoka@redhat.com/T/
- As Simon pointed out, the valid packets will reach invalid_pkt_free
and kfree_skb(skb) after being handled correctly in switch statement.
It can lead to double free issues, which is not intended. So this patch
uses "continue" instead of "break" in switch statement.

- In the current implementation, once zero payload size is detected, the
for statement exits. It should continue processing subsequent packets. 
So this patch just frees skb in invalid_pkt_free when the invalid 
packets are detected. [3]

v2
https://lore.kernel.org/lkml/20240428134525.GW516117@kernel.org/T/

- The v1 patch only checked whether skb->len is zero. This patch also
  checks header size, payload size and total packet size.


v1
https://lore.kernel.org/linux-kernel/CANn89iJrQevxPFLCj2P=U+XSisYD0jqrUQpa=zWMXTjj5+RriA@mail.gmail.com/T/

 net/nfc/nci/core.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index b133dc55304c..0aaff30cb68f 100644
--- a/net/nfc/nci/core.c
+++ b/net/nfc/nci/core.c
@@ -1463,6 +1463,16 @@ int nci_core_ntf_packet(struct nci_dev *ndev, __u16 opcode,
 				 ndev->ops->n_core_ops);
 }
 
+static bool nci_valid_size(struct sk_buff *skb, unsigned int header_size)
+{
+	if (skb->len < header_size ||
+	    !nci_plen(skb->data) ||
+	    skb->len < header_size + nci_plen(skb->data)) {
+		return false;
+	}
+	return true;
+}
+
 /* ---- NCI TX Data worker thread ---- */
 
 static void nci_tx_work(struct work_struct *work)
@@ -1516,24 +1526,32 @@ 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) {
 			kfree_skb(skb);
-			kcov_remote_stop();
-			break;
+			continue;
 		}
 
 		/* Process frame */
 		switch (nci_mt(skb->data)) {
 		case NCI_MT_RSP_PKT:
-			nci_rsp_packet(ndev, skb);
+			if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
+				nci_rsp_packet(ndev, skb);
+			else
+				kfree_skb(skb);
 			break;
 
 		case NCI_MT_NTF_PKT:
-			nci_ntf_packet(ndev, skb);
+			if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
+				nci_ntf_packet(ndev, skb);
+			else
+				kfree_skb(skb);
 			break;
 
 		case NCI_MT_DATA_PKT:
-			nci_rx_data_packet(ndev, skb);
+			if (nci_valid_size(skb, NCI_DATA_HDR_SIZE))
+				nci_rx_data_packet(ndev, skb);
+			else
+				kfree_skb(skb);
 			break;
 
 		default:
-- 
2.44.0

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

[Simon Horman]

On Thu, May 09, 2024 at 08:30:33PM +0900, Ryosuke Yasuoka wrote:
> syzbot reported the following uninit-value access issue [1]
> 
> nci_rx_work() parses received packet from ndev->rx_q. It should be
> validated header size, payload size and total packet size before
> processing the packet. If an invalid packet is detected, it 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>

I suggest giving time for Krzysztof to review this.
But from my side this looks good.

Reviewed-by: Simon Horman <horms@kernel.org>

...

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

[Krzysztof Kozlowski]

On 09/05/2024 13:30, Ryosuke Yasuoka wrote:
> syzbot reported the following uninit-value access issue [1]
> 
> nci_rx_work() parses received packet from ndev->rx_q. It should be
> validated header size, payload size and total packet size before
> processing the packet. If an invalid packet is detected, it 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>
> ---
> v4
> - v3 patch uses goto statement and it makes codes complicated. So this
>   patch simply calls kfree_skb inside loop and remove goto statement.
> - [2] inserted kcov_remote_stop() to fix kcov check. However, as we
>   discuss about my v3 patch [3], it should not exit the for statement
>   and should continue processing subsequent packets. This patch removes


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Jakub Kicinski]

On Thu,  9 May 2024 20:30:33 +0900 Ryosuke Yasuoka wrote:
> -		if (!nci_plen(skb->data)) {
> +		if (!skb->len) {
>  			kfree_skb(skb);
> -			kcov_remote_stop();
> -			break;
> +			continue;

the change from break to continue looks unrelated

>  		}

> -			nci_ntf_packet(ndev, skb);
> +			if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE))

> +			if (nci_valid_size(skb, NCI_DATA_HDR_SIZE))


#define NCI_CTRL_HDR_SIZE                                       3
#define NCI_DATA_HDR_SIZE                                       3

you can add a BUILD_BUG_ON(NCI_CTRL_HDR_SIZE == NCI_DATA_HDR_SIZE)
and save all the code duplication.

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

[Ryosuke Yasuoka]

Thank you for your review.

On Fri, May 10, 2024 at 07:06:13PM -0700, Jakub Kicinski wrote:
> On Thu,  9 May 2024 20:30:33 +0900 Ryosuke Yasuoka wrote:
> > -		if (!nci_plen(skb->data)) {
> > +		if (!skb->len) {
> >  			kfree_skb(skb);
> > -			kcov_remote_stop();
> > -			break;
> > +			continue;
> 
> the change from break to continue looks unrelated

OK. I'll leave this break in this patch. I'll send another patch about
it.

> >  		}
> 
> > -			nci_ntf_packet(ndev, skb);
> > +			if (nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> 
> > +			if (nci_valid_size(skb, NCI_DATA_HDR_SIZE))
> 
> 
> #define NCI_CTRL_HDR_SIZE                                       3
> #define NCI_DATA_HDR_SIZE                                       3
> 
> you can add a BUILD_BUG_ON(NCI_CTRL_HDR_SIZE == NCI_DATA_HDR_SIZE)
> and save all the code duplication.
> 

Sorry I don't get it. Do you mean I just insert
BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE) or insert this and
clean up the code duplication like this? (It is just a draft. I just
share what I mean.) I can avoid to call nci_valid_size() repeatedly
inside the switch statement.

static void nci_rx_work(struct work_struct *work)
{
...
		if (!skb->len) {
			kfree_skb(skb);
			kcov_remote_stop();
			break;
		}

		BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE);
		unsigned int hdr_size = NCI_CTRL_HDR_SIZE;

		if (!nci_valid_size(skb, hdr_size)) {
			kfree_skb(skb);
			continue;
		}

		/* Process frame */
		switch (nci_mt(skb->data)) {
		case NCI_MT_RSP_PKT:
			nci_rsp_packet(ndev, skb);
			break;

		case NCI_MT_NTF_PKT:
			nci_ntf_packet(ndev, skb);
			break;

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

[Jakub Kicinski]

On Sat, 11 May 2024 20:02:28 +0900 Ryosuke Yasuoka wrote:
> Sorry I don't get it. Do you mean I just insert
> BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE) or insert this and
> clean up the code duplication like this? (It is just a draft. I just
> share what I mean.) I can avoid to call nci_valid_size() repeatedly
> inside the switch statement.
> 
> static void nci_rx_work(struct work_struct *work)
> {
> ...
> 		if (!skb->len) {
> 			kfree_skb(skb);
> 			kcov_remote_stop();
> 			break;
> 		}
> 
> 		BUILD_BUG_ON(NCI_CTRL_HDR_SIZE != NCI_DATA_HDR_SIZE);
> 		unsigned int hdr_size = NCI_CTRL_HDR_SIZE;
> 
> 		if (!nci_valid_size(skb, hdr_size)) {
> 			kfree_skb(skb);
> 			continue;
> 		}
> 
> 		/* Process frame */
> 		switch (nci_mt(skb->data)) {
> 		case NCI_MT_RSP_PKT:
> 			nci_rsp_packet(ndev, skb);
> 			break;
> 
> 		case NCI_MT_NTF_PKT:
> 			nci_ntf_packet(ndev, skb);
> 			break;

Yes, that's what I meant. I'd probably merge the nci_valid_size()
check with the !skb->len check, too.