Re: [PATCH net] nfc: nci: avoid unbounded skb allocation when max_pkt_payload_len is zero

[Simon Horman <horms@kernel.org>]

On Mon, Sep 14, 2026 at 11:08:34AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0

Hi Lin,

There is an expectation that AI-generated reviews forwarded to
the netdev ML are responded to. That can take a variety of forms
including an email response that agrees with or refutes the review,
or an updated version of the patch with an appropriate changelog.

The expectation is the patch author or submitter handles this.
But let me try to help by taking a stab at the first approach.

> - [High] Incomplete fix: a controller-supplied
>   `conn_info->max_pkt_payload_len` of 0 (or 1) is still consumed…
> - [High] TOCTOU: the new guard reads `conn_info->max_pkt_payload_len`
>   once (net/nfc/nci/data.c:117) but the fragmentation loop re-reads the…

It seems to me that both of the above are actually a pre-existing issues
that are not made worse by this patch.

> 
> Pre-existing issues:
> - [High] Use-after-free of `struct nci_conn_info`:
>   `nci_get_conn_info_by_conn_id()` (net/nfc/nci/core.c:43-54) walks…

It seems to me that although this is a pre-existing issue it may
actually be made worse than this patch.

Overall, my view is that bug fixes for NFC improve it's quality,
moving things in the right direction. I lean towards none of
the above warranting blocking the progress of this patch.
But I would appreciate your input.

> 
> --
> 
> nfc: nci: avoid unbounded skb allocation when max_pkt_payload_len is zero
> 
> This adds a check in nci_queue_tx_data_frags() that rejects a
> conn_info->max_pkt_payload_len of zero with -EPROTO, so the fragmentation
> loop cannot spin forever allocating skbs.  The commit message explains why
> the check was placed in the fragmentation path instead of at the two
> assignment sites in ntf.c and rsp.c.
> 
> A few questions below about the placement of the check and about how the
> field is read.
> 
> > diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
> > index 4253edea5c8db..b549cef7dc2e4 100644
> > --- a/net/nfc/nci/data.c
> > +++ b/net/nfc/nci/data.c
> > @@ -114,6 +114,11 @@ static int nci_queue_tx_data_frags(struct nci_dev *ndev,
> >  		goto exit;
> >  	}
> >  
> > +	if (!conn_info->max_pkt_payload_len) {
> > +		rc = -EPROTO;
> > +		goto exit;
> > +	}
> > +
> 
> [Severity: High]
> The commit message says:
> 
>   "nci_queue_tx_data_frags() is the only place that loops"
> 
> Is that accurate?  nci_hci_send_data() in net/nfc/nci/hci.c also loops over
> the same field, and it runs before nci_send_data() gets a chance to reject
> anything:
> 
> net/nfc/nci/hci.c:nci_hci_send_data() {
> 	skb = nci_skb_alloc(ndev, conn_info->max_pkt_payload_len +
> 			    NCI_DATA_HDR_SIZE, GFP_ATOMIC);
> 	...
> 	skb_reserve(skb, NCI_DATA_HDR_SIZE + 2);
> 	*(u8 *)skb_push(skb, 1) = data_type;
> 
> 	do {
> 		/* If last packet add NCI_HFP_NO_CHAINING */
> 		if (i + conn_info->max_pkt_payload_len -
> 		    (skb->len + 1) >= data_len) {
> 			cb |= NCI_HFP_NO_CHAINING;
> 			len = data_len - i;
> 		} else {
> 			len = conn_info->max_pkt_payload_len - skb->len - 1;
> 		}
> 
> 		*(u8 *)skb_push(skb, 1) = cb;
> 
> 		if (len > 0)
> 			skb_put_data(skb, data + i, len);
> 
> 		r = nci_send_data(ndev, conn_info->conn_id, skb);
> 	...
> }
> 
> skb->len is unsigned int, so i + max_pkt_payload_len - (skb->len + 1) is
> evaluated unsigned.  With max_pkt_payload_len == 0, i == 0 and skb->len == 1
> that is 0 - 2 == 0xfffffffe, which is >= any realistic data_len, so the
> "last packet" branch is always taken and len becomes the whole payload.  The
> same underflow happens for max_pkt_payload_len == 1.
> 
> The skb was only sized for 0 + NCI_DATA_HDR_SIZE plus tx_headroom/tx_tailroom,
> so the usable tailroom is just the kmalloc/SKB_DATA_ALIGN slack.  Can
> skb_put_data() then run past skb->end and hit skb_over_panic() -> BUG()
> before the new check in nci_queue_tx_data_frags() is ever consulted?
> 
> The value fed into that arithmetic is stored verbatim in
> nci_core_conn_create_rsp_packet():
> 
> net/nfc/nci/rsp.c:nci_core_conn_create_rsp_packet() {
> 	...
> 	if (ndev->cur_params.id == ndev->hci_dev->nfcee_id)
> 		ndev->hci_dev->conn_info = conn_info;
> 
> 	conn_info->conn_id = rsp->conn_id;
> 	conn_info->max_pkt_payload_len = rsp->max_ctrl_pkt_payload_len;
> 	...
> }
> 
> and the payload length on the other side is user controlled:
> nfc_genl_se_io() takes NFC_ATTR_SE_APDU as NLA_BINARY with no maximum, and
> st_nci_se_io() -> nci_hci_send_event() -> nci_hci_send_data() carries it
> straight into the loop above.  nci_hci_cmd_received() also calls
> nci_hci_send_data() directly from the rx work.
> 
> Would it make more sense to validate at the assignment sites
> (nci_core_conn_create_rsp_packet() and nci_rf_intf_activated_ntf_packet()),
> or to additionally fix the arithmetic in nci_hci_send_data()?  A plain
> != 0 test would still leave the max_pkt_payload_len == 1 underflow in place.
> 
> [Severity: High]
> The new check reads conn_info->max_pkt_payload_len once, but the loop below
> re-reads the same field on every iteration:
> 
> net/nfc/nci/data.c:nci_queue_tx_data_frags() {
> 	...
> 	while (total_len) {
> 		frag_len =
> 			min_t(int, total_len, conn_info->max_pkt_payload_len);
> 	...
> }
> 
> The field is a plain __u8 with no READ_ONCE/WRITE_ONCE, no atomic and no
> lock, and it is written from the rx workqueue with controller-supplied data
> in nci_rf_intf_activated_ntf_packet() and nci_core_conn_create_rsp_packet().
> 
> The reader side is rawsock_tx_work -> nfc_data_exchange() -> nci_transceive()
> -> nci_send_data() -> nci_queue_tx_data_frags(); the only lock it holds is
> device_lock(&dev->dev), plus ndev->tx_q.lock after the loop.  The writer side
> is nci_rx_work() -> nci_ntf_packet()/nci_rsp_packet() -> the assignments, and
> neither takes device_lock.
> 
> CPU0 (rawsock_tx_work)                        CPU1 (nci_rx_work)
> conn_info = nci_get_conn_info_by_conn_id()
> if (!conn_info->max_pkt_payload_len)  /* nonzero, passes */
> frag_len = min_t(int, total_len,
> 		 conn_info->max_pkt_payload_len);
>                                               conn_info->max_pkt_payload_len = 0;
> /* next iteration */
> frag_len = 0 -> total_len never decreases
> 
> Can the store land between the check and a later re-read, leaving the loop in
> exactly the unbounded nci_skb_alloc(GFP_ATOMIC) state the commit describes?
> 
> Would snapshotting the field into a local, e.g.
> 
> 	u8 max_len = READ_ONCE(conn_info->max_pkt_payload_len);
> 
> and using that for both the validation and the min_t() close the window?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the conn_info pointer that the
> new check dereferences has no lifetime guarantee.
> nci_get_conn_info_by_conn_id() walks the list with no lock, no RCU and no
> lockdep assertion:
> 
> net/nfc/nci/core.c:nci_get_conn_info_by_conn_id() {
> 	list_for_each_entry(conn_info, &ndev->conn_info_list, list) {
> 		if (conn_info->conn_id == conn_id)
> 			return conn_info;
> 	}
> 
> 	return NULL;
> }
> 
> while the rx work frees the same object immediately, with no grace period:
> 
> net/nfc/nci/rsp.c:nci_core_conn_close_rsp_packet() {
> 	if (status == NCI_STATUS_OK) {
> 		conn_info = nci_get_conn_info_by_conn_id(ndev,
> 							 ndev->cur_conn_id);
> 		if (conn_info) {
> 			list_del(&conn_info->list);
> 			if (conn_info == ndev->rf_conn_info)
> 				ndev->rf_conn_info = NULL;
> 			devm_kfree(&ndev->nfc_dev->dev, conn_info->dest_params);
> 			devm_kfree(&ndev->nfc_dev->dev, conn_info);
> 		}
> 	}
> 	...
> }
> 
> nci_rsp_packet() dispatches CORE_CONN_CLOSE_RSP without checking that a
> matching command was outstanding, and the tx path holds only
> device_lock(&dev->dev), which the rx worker never takes.  Can an unsolicited
> CORE_CONN_CLOSE_RSP with status OK for ndev->cur_conn_id free the conn_info
> while nci_queue_tx_data_frags() is still reading
> conn_info->max_pkt_payload_len, both in the new check and in the min_t() in
> the loop?  The list walk itself can also follow a freed node's linkage.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913101309.891633-1-liuc63%40xiaopeng.com