diff mbox series

[net,v3] nfc: nci: Fix uninit-value in nci_rx_work

Message ID 20240502082323.250739-1-ryasuoka@redhat.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,v3] nfc: nci: Fix uninit-value in nci_rx_work | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 927 this patch: 927
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 1 blamed authors not CCed: jeremy@jcline.org; 1 maintainers not CCed: jeremy@jcline.org
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 938 this patch: 938
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 60 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-05-03--15-00 (tests: 1001)

Commit Message

Ryosuke Yasuoka May 2, 2024, 8:22 a.m. UTC
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>
---

v3
- 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.

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 | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

Krzysztof Kozlowski May 3, 2024, 9:07 a.m. UTC | #1
On 02/05/2024 10:22, 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>
> ---
> 
> v3
> - 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.
> 
> 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 | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> index 0d26c8ec9993..e4f92a090022 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,35 @@ 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;
> +			continue;

I don't find this code readable.

>  
>  		case NCI_MT_NTF_PKT:
> +			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> +				goto invalid_pkt_free;
>  			nci_ntf_packet(ndev, skb);
> -			break;
> +			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);
> -			break;
> +			continue;
>  
>  		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);

Why you cannot kfree in "default" and error cases? I don't think that
goto inside loop makes this code easier to follow.

>  	}
>  
>  	/* check if a data exchange timeout has occurred */

Best regards,
Krzysztof
Ryosuke Yasuoka May 4, 2024, 4:33 p.m. UTC | #2
On Fri, May 03, 2024 at 11:07:49AM +0200, Krzysztof Kozlowski wrote:
> On 02/05/2024 10:22, 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>
> > ---
> > 
> > v3
> > - 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.
> > 
> > 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 | 33 ++++++++++++++++++++++++---------
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> > index 0d26c8ec9993..e4f92a090022 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,35 @@ 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;
> > +			continue;
> 
> I don't find this code readable.
> 
> >  
> >  		case NCI_MT_NTF_PKT:
> > +			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
> > +				goto invalid_pkt_free;
> >  			nci_ntf_packet(ndev, skb);
> > -			break;
> > +			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);
> > -			break;
> > +			continue;
> >  
> >  		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);
> 
> Why you cannot kfree in "default" and error cases? I don't think that
> goto inside loop makes this code easier to follow.

Thank you for your review, Krzysztof.

Yes, we can write this without goto statement. But if we don't use goto
statement, we have to call kfree_skb() and break in each switch
statement 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) {
			kfree_skb(skb);   <<<---
			continue;   <<<---
		}

		/* Process frame */
		switch (nci_mt(skb->data)) {
		case NCI_MT_RSP_PKT:
			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) {
				kfree_skb(skb);   <<<---
				break;   <<<---
			}
			nci_rsp_packet(ndev, skb);
			break;

		case NCI_MT_NTF_PKT:
			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) {
				kfree_skb(skb);   <<<---
				break;   <<<---
			}
			nci_ntf_packet(ndev, skb);
			break;

		case NCI_MT_DATA_PKT:
			if (!nci_valid_size(skb, NCI_DATA_HDR_SIZE)) {
				kfree_skb(skb);   <<<---
				break;   <<<---
			}
			nci_rx_data_packet(ndev, skb);
			break;

		default:
			pr_err("unknown MT 0x%x\n", nci_mt(skb->data));
			kfree_skb(skb);   <<<---
			break;   <<<---
		}
	}

IMHO, using goto statement can avoid calling this statement repeatedly,
and it might make these codes brief. I understand goto statement often
makes codes complicated as you mention, So please let me know again if I
should write these codes without goto. I'd like to respect your idea and
I'm willing to send v4 patch.

Thank you,
Ryosuke
diff mbox series

Patch

diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
index 0d26c8ec9993..e4f92a090022 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,35 @@  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;
+			continue;
 
 		case NCI_MT_NTF_PKT:
+			if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE))
+				goto invalid_pkt_free;
 			nci_ntf_packet(ndev, skb);
-			break;
+			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);
-			break;
+			continue;
 
 		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);
 	}
 
 	/* check if a data exchange timeout has occurred */