[PATCH net] nfc: nci: reject unusable max payload limits

[PATCH net] nfc: nci: reject unusable max payload limits

[Liu Chao]

nci_core_conn_create_rsp_packet() copies the controller-supplied
max_ctrl_pkt_payload_len into the new connection without any
validation, and nci_hci_send_data() sizes its fragments from it.
When the controller reports 0 or 1, the subtraction in the loop
underflows (skb->len is unsigned), the "last packet" branch is
always taken, and skb_put_data() runs past skb->end into
skb_over_panic(). A limit of 2 still works: the first packet
carries the two HCP header bytes, and every chained packet one
payload byte.

Reject a zero limit at parse time, before the conn_info is
allocated and published, so there is nothing to unwind. A
zero-payload connection cannot carry any data anyway, and both
in-tree creators start sending right after the connection comes
up: st-nci sets up its HCI session, fdp downloads firmware
through the generic data path.

A limit of 1 stays legal at the NCI layer, since
nci_queue_tx_data_frags() can ship one-byte fragments over such
a connection. Only HCI needs two bytes for the HCP header, so
that check lives in nci_hci_send_data(), which snapshots the
limit the same way nci_queue_tx_data_frags() does and returns
-EPROTO below 2.

Fixes: 4aeee6871e8c ("NFC: nci: Add dynamic logical connections support")
Fixes: 11f54f228643 ("NFC: nci: Add HCI over NCI protocol support")
Cc: stable@vger.kernel.org
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
Link: https://lore.kernel.org/netdev/20260918185458.2711284-1-liuc63@xiaopeng.com
---
 net/nfc/nci/hci.c | 22 +++++++++++++++++-----
 net/nfc/nci/rsp.c |  9 +++++++++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
index c03e8a0bd..d68fe55b6 100644
--- a/net/nfc/nci/hci.c
+++ b/net/nfc/nci/hci.c
@@ -144,6 +144,7 @@ static int nci_hci_send_data(struct nci_dev *ndev, u8 pipe,
 			     size_t data_len)
 {
 	const struct nci_conn_info *conn_info;
+	u8 max_pkt_payload_len;
 	struct sk_buff *skb;
 	int len, i, r;
 	u8 cb = pipe;
@@ -152,8 +153,20 @@ static int nci_hci_send_data(struct nci_dev *ndev, u8 pipe,
 	if (!conn_info)
 		return -EPROTO;
 
+	/* Snapshot the limit like nci_queue_tx_data_frags() does; the
+	 * conn_info is published before this field is written.
+	 */
+	max_pkt_payload_len = READ_ONCE(conn_info->max_pkt_payload_len);
+
+	/* Below 2 the unsigned fragment arithmetic wraps and the first
+	 * skb_put_data() runs past skb->end; 2 is the smallest working
+	 * limit.
+	 */
+	if (max_pkt_payload_len < 2)
+		return -EPROTO;
+
 	i = 0;
-	skb = nci_skb_alloc(ndev, conn_info->max_pkt_payload_len +
+	skb = nci_skb_alloc(ndev, max_pkt_payload_len +
 			    NCI_DATA_HDR_SIZE, GFP_ATOMIC);
 	if (!skb)
 		return -ENOMEM;
@@ -163,12 +176,11 @@ static int nci_hci_send_data(struct nci_dev *ndev, u8 pipe,
 
 	do {
 		/* If last packet add NCI_HFP_NO_CHAINING */
-		if (i + conn_info->max_pkt_payload_len -
-		    (skb->len + 1) >= data_len) {
+		if (i + 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;
+			len = max_pkt_payload_len - skb->len - 1;
 		}
 
 		*(u8 *)skb_push(skb, 1) = cb;
@@ -184,7 +196,7 @@ static int nci_hci_send_data(struct nci_dev *ndev, u8 pipe,
 
 		if (i < data_len) {
 			skb = nci_skb_alloc(ndev,
-					    conn_info->max_pkt_payload_len +
+					    max_pkt_payload_len +
 					    NCI_DATA_HDR_SIZE, GFP_ATOMIC);
 			if (!skb)
 				return -ENOMEM;
diff --git a/net/nfc/nci/rsp.c b/net/nfc/nci/rsp.c
index b0ab4f5ac..70aafe0da 100644
--- a/net/nfc/nci/rsp.c
+++ b/net/nfc/nci/rsp.c
@@ -314,6 +314,15 @@ static void nci_core_conn_create_rsp_packet(struct nci_dev *ndev,
 	if (status == NCI_STATUS_OK) {
 		rsp = (struct nci_core_conn_create_rsp *)skb->data;
 
+		/* A zero payload limit cannot carry any data; reject before
+		 * the conn_info is published. A limit of 1 still works for
+		 * one-byte generic data fragments.
+		 */
+		if (!rsp->max_ctrl_pkt_payload_len) {
+			status = NCI_STATUS_REJECTED;
+			goto exit;
+		}
+
 		conn_info = devm_kzalloc(&ndev->nfc_dev->dev,
 					 sizeof(*conn_info), GFP_KERNEL);
 		if (!conn_info) {
-- 
2.50.1

Re: [PATCH net] nfc: nci: reject unusable max payload limits

[Simon Horman]

On Sat, Sep 19, 2026 at 01:49:31PM +0800, Liu Chao wrote:
> nci_core_conn_create_rsp_packet() copies the controller-supplied
> max_ctrl_pkt_payload_len into the new connection without any
> validation, and nci_hci_send_data() sizes its fragments from it.
> When the controller reports 0 or 1, the subtraction in the loop
> underflows (skb->len is unsigned), the "last packet" branch is
> always taken, and skb_put_data() runs past skb->end into
> skb_over_panic(). A limit of 2 still works: the first packet
> carries the two HCP header bytes, and every chained packet one
> payload byte.
> 
> Reject a zero limit at parse time, before the conn_info is
> allocated and published, so there is nothing to unwind. A
> zero-payload connection cannot carry any data anyway, and both
> in-tree creators start sending right after the connection comes
> up: st-nci sets up its HCI session, fdp downloads firmware
> through the generic data path.
> 
> A limit of 1 stays legal at the NCI layer, since
> nci_queue_tx_data_frags() can ship one-byte fragments over such
> a connection. Only HCI needs two bytes for the HCP header, so
> that check lives in nci_hci_send_data(), which snapshots the
> limit the same way nci_queue_tx_data_frags() does and returns
> -EPROTO below 2.
> 
> Fixes: 4aeee6871e8c ("NFC: nci: Add dynamic logical connections support")
> Fixes: 11f54f228643 ("NFC: nci: Add HCI over NCI protocol support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
> Link: https://lore.kernel.org/netdev/20260918185458.2711284-1-liuc63@xiaopeng.com

Hi Liu,

I am wondering if this patch is intended as an alternative or supplement
to your patch at the link above.

If it is an alternative then I'd appreciate some clarification of how
the following point in the cover letter of the patch at the link above
is addressed:

  Reject the zero value in the fragmentation path rather than at the
  assignment sites.  nci_queue_tx_data_frags() is the only place that
  loops over the RF data path's conn_info, and nci_send_data() takes
  the non-fragmenting branch only for skb->len <= max_pkt_payload_len,
  which for a zero limit means empty skbs alone.  Validating on
  assignment would not be sufficient either, because
  nci_rf_disc_rsp_packet() allocates ndev->rf_conn_info with
  devm_kzalloc(), so max_pkt_payload_len is already zero before any
  notification arrives.

In particular the "not be sufficient" part.

And regardless, I have a concern, which is touched on in the AI-generated
review I passed on in my response to the patch at the link above.

My concern is that conn_info->max_pkt_payload_len is read by various
functions in the call chain leading to nci_queue_tx_data_frags(). But it
can change at any time, leading to possible inconsistencies.

I have not analysed what they are or if they manifest in bugs.
But I do wonder if a single snapshot of max_pkt_payload_len needs
to be taken and passed down the call-chain.

...