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

[PATCH net v2] 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>
---
v2
- 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 | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 0d26c8ec9993..ab07b5f69664 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,30 +1526,36 @@ 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)) {
-			kfree_skb(skb);
-			break;
-		}
+		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);
 			break;
 
 		case NCI_MT_NTF_PKT:
+			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
+				goto invalid_pkt_free;
 			nci_ntf_packet(ndev, skb);
 			break;
 
 		case NCI_MT_DATA_PKT:
+			if (!nci_valid_size(skb, NCI_DATA_HDR_SIZE))
+				goto invalid_pkt_free;
 			nci_rx_data_packet(ndev, skb);
 			break;
 
 		default:
 			pr_err("unknown MT 0x%x\n", nci_mt(skb->data));
-			kfree_skb(skb);
-			break;
+			goto invalid_pkt_free;
 		}
+invalid_pkt_free:
+		kfree_skb(skb);
+		break;
 	}
 
 	/* check if a data exchange timeout has occurred */
-- 
2.44.0

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

[Simon Horman]

On Sat, Apr 27, 2024 at 07:35:54PM +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>

...

> @@ -1516,30 +1526,36 @@ 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)) {
> -			kfree_skb(skb);
> -			break;
> -		}
> +		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;

Hi Yasuoka-san,

My reading is that this will jump to the label invalid_pkt_free,
which is intended.

>  			nci_rsp_packet(ndev, skb);
>  			break;

But this will exit the switch statement, which lands
at the label invalid_pkt_free, where skb is kfreed.
This doesn't seem to be intended.

>  
>  		case NCI_MT_NTF_PKT:
> +			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> +				goto invalid_pkt_free;
>  			nci_ntf_packet(ndev, skb);
>  			break;
>  
>  		case NCI_MT_DATA_PKT:
> +			if (!nci_valid_size(skb, NCI_DATA_HDR_SIZE))
> +				goto invalid_pkt_free;
>  			nci_rx_data_packet(ndev, skb);
>  			break;
>  
>  		default:
>  			pr_err("unknown MT 0x%x\n", nci_mt(skb->data));
> -			kfree_skb(skb);
> -			break;
> +			goto invalid_pkt_free;
>  		}

If so, then one solution may be to add the following here:

		continue;

> +invalid_pkt_free:
> +		kfree_skb(skb);
> +		break;
>  	}
>  
>  	/* check if a data exchange timeout has occurred */
> -- 
> 2.44.0
> 
> 

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

[Ryosuke Yasuoka]

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:
> > 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>
> 
> ...
> 
> > @@ -1516,30 +1526,36 @@ 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)) {
> > -			kfree_skb(skb);
> > -			break;
> > -		}
> > +		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;
> 
> Hi Yasuoka-san,
> 
> My reading is that this will jump to the label invalid_pkt_free,
> which is intended.
> 
> >  			nci_rsp_packet(ndev, skb);
> >  			break;
> 
> But this will exit the switch statement, which lands
> at the label invalid_pkt_free, where skb is kfreed.
> This doesn't seem to be intended.
> 
> >  
> >  		case NCI_MT_NTF_PKT:
> > +			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> > +				goto invalid_pkt_free;
> >  			nci_ntf_packet(ndev, skb);
> >  			break;
> >  
> >  		case NCI_MT_DATA_PKT:
> > +			if (!nci_valid_size(skb, NCI_DATA_HDR_SIZE))
> > +				goto invalid_pkt_free;
> >  			nci_rx_data_packet(ndev, skb);
> >  			break;
> >  
> >  		default:
> >  			pr_err("unknown MT 0x%x\n", nci_mt(skb->data));
> > -			kfree_skb(skb);
> > -			break;
> > +			goto invalid_pkt_free;
> >  		}
> 
> If so, then one solution may be to add the following here:
> 
> 		continue;
> 
> > +invalid_pkt_free:
> > +		kfree_skb(skb);
> > +		break;
> >  	}
> >  
> >  	/* check if a data exchange timeout has occurred */
> > -- 
> > 2.44.0
> > 
> > 
> 

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?

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

[Simon Horman]

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