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

[Ryosuke Yasuoka <ryasuoka@redhat.com>]

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