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

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

[Liu Chao]

nci_queue_tx_data_frags() uses conn_info->max_pkt_payload_len as the
fragment size.  When that value is zero, frag_len is always zero and
total_len never decreases.  The loop then allocates skbs without bound:
none of them are freed inside the loop, they accumulate on frags_q, and
there is no cond_resched() in the loop body.  A single sendmsg() can
therefore consume all allocatable memory, and on CONFIG_PREEMPT_NONE it
occupies the CPU long enough to trip the softlockup watchdog:

  watchdog: BUG: soft lockup - CPU#3 stuck for 26s! [kworker/3:1:57]
  Workqueue: events rawsock_tx_work [nfc]
  Call Trace:
   nci_send_data+0x1ca/0x6b0 [nci]
   nci_transceive+0xbb/0x170 [nci]
   rawsock_tx_work+0xb5/0x1a0 [nfc]

max_pkt_payload_len is taken verbatim from controller-supplied fields,
with no check for zero:

  ntf.c: conn_info->max_pkt_payload_len = ntf.max_data_pkt_payload_size;
  rsp.c: conn_info->max_pkt_payload_len = rsp->max_ctrl_pkt_payload_len;

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

No legitimate configuration is known to be affected.  Where the NCI
spec does mandate a zero Max Data Packet Payload Size -- the NFCEE
Direct RF Interface -- nci_rf_intf_activated_ntf_packet() takes the
"goto listen" shortcut, bypassing the assignment entirely.

Reproduced with CONFIG_NFC_VIRTUAL_NCI by injecting an
RF_INTF_ACTIVATED_NTF with max_data_pkt_payload_size set to 0 and then
sending a data frame on an AF_NFC SEQPACKET socket.

Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
Cc: stable@vger.kernel.org
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
 net/nfc/nci/data.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
index 4253edea5..b549cef7d 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;
+	}
+
 	__skb_queue_head_init(&frags_q);
 
 	while (total_len) {

base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
-- 
2.50.1

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 2 (1) · Medium: 0 · Low: 0

- [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…

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…

--

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

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

[Simon Horman]

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

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

[Liu Chao]

Hi Simon,

Thanks for stepping in. Answering this is on me, and v2, posted
right after this mail, covers the other form.

To your question: no, I don't think any of the three should block
this patch. The TOCTOU race and the conn_info lifetime problem are
pre-existing and this patch widens neither. I'd rather fix them in
follow-ups than fold a lifetime fix into a 5-line bounds check.

On the UAF, you wrote that it "may actually be made worse" by
this patch. I don't see how. The guard adds no dereference the
loop doesn't already perform: with the check removed, the next
iteration still reads the same field through the same unlocked
pointer, and the list walk is unlocked either way. If you have a
concrete window in mind, please spell it out.

> - [High] Incomplete fix: a controller-supplied
>   conn_info->max_pkt_payload_len of 0 (or 1) is still consumed by
>   nci_hci_send_data() in net/nfc/nci/hci.c ...

Agreed, pre-existing and not made worse by this patch. Sashiko is
right that "nci_queue_tx_data_frags() is the only place that loops"
was too broad; nci_hci_send_data() loops over the same field. Two
scope facts, though: the RF path uses ndev->rf_conn_info (allocated
in nci_rf_disc_rsp_packet(), limit set from ntf.max_data_pkt_payload_size
in nci_rf_intf_activated_ntf_packet()), while the HCI path uses
ndev->hci_dev->conn_info (allocated in
nci_core_conn_create_rsp_packet(), limit set from
rsp->max_ctrl_pkt_payload_len). Different objects, so the patch
doesn't miss the bug it fixes, but the sentence oversold it. v2
rewords it.

The new check can still reject a zero on the HCI path, but that
buys little. For non-empty payloads the unsigned underflow in the
loop above drives len past skb->end into skb_over_panic() -> BUG()
before nci_send_data() is ever called, and what does reach
nci_send_data() gets there only after skb_put_data() has run. So
the path needs its own fix.

That fix belongs where the response is parsed, before the conn_info
is published. rsp->max_ctrl_pkt_payload_len is available as soon as
rsp is set up (rsp.c:315); the object is allocated after that, and
is on ndev->conn_info_list (rsp.c:342) and installed as
ndev->hci_dev->conn_info (rsp.c:345) before the field is written
(rsp.c:348). A check at parse time means the object is never
published, nothing to unwind. A check at the assignment comes too
late: the object is already linked and already pointed at, and the
existing error path (free_conn_info, rsp.c:352) only covers the
pre-list_add allocation failure, with no list_del and no clearing
of hci_dev->conn_info. A zero there can only be the controller
reporting a broken limit, so rejecting the response outright is
the right call.

The RF path is the opposite case. nci_rf_disc_rsp_packet()
devm_kzallocs rf_conn_info, so zero is its legitimate initial
state, the "not yet activated" value, not something the controller
reported. The activation notification can't cover that window, and
rejecting a zero there only restores it for the next transmitter.
The reproducer walks exactly this path: RF_DISCOVER_RSP creates
rf_conn_info, the injected ACTIVATED_NTF stores zero, the target
still activates, the next data frame spins.

The entry path doesn't help either. nci_send_data() takes the
non-fragmenting branch only for skb->len <= max_pkt_payload_len,
so with a zero limit a non-empty frame reaches the fragmentation
loop no matter what the writers do. The check at the point of use
covers every producer of a zero limit: initial state, the
notification, any future writer. That's why it lives there for RF
and won't for HCI.

The arithmetic in nci_hci_send_data() needs its own fix regardless.
With max_pkt_payload_len 0 or 1, i + max_pkt_payload_len -
(skb->len + 1) is evaluated unsigned and wraps, the "last packet"
branch is always taken, len becomes the whole payload, and
skb_put_data() runs past skb->end into skb_over_panic() -> BUG()
instead of merely spinning. A plain != 0 test wouldn't cover that.
I'll send the arithmetic fix plus the parse-time zero check for
that path.

> - [High] TOCTOU: the new guard reads conn_info->max_pkt_payload_len
>   once but the fragmentation loop re-reads it on every iteration.

Agreed, the window is real and pre-existing. I've folded Sashiko's
snapshot into v2 rather than deferring it: the loop re-reads the
field either way, so the race itself is old, but the new check
shouldn't be bypassable by the store it guards. v2 snapshots the
field once with READ_ONCE() and uses the snapshot for both the
check and the min_t() bound.

> Pre-existing issues:
> - [High] Use-after-free of struct nci_conn_info ...

Agreed. Unlocked list walk, devm_kfree() straight from the rx
worker, and nci_rsp_packet() dispatching CORE_CONN_CLOSE_RSP with
no outstanding command. The HCI side has the same shape:
nci_core_conn_close_rsp_packet() clears ndev->rf_conn_info but
leaves ndev->hci_dev->conn_info (written only at rsp.c:345, never
cleared) pointing at the freed object. I'll do the lifetime work
as its own series rather than smuggle it into a 5-line bounds fix.

v2 follows this mail with the changelog correction and the
READ_ONCE() snapshot; the HCI loop gets its own patch after that,
then the lifetime series.

Thanks,

Liu Chao

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

[Liu Chao]

nci_queue_tx_data_frags() uses conn_info->max_pkt_payload_len as the
fragment size.  When that value is zero, frag_len is always zero and
total_len never decreases.  The loop then allocates skbs without
bound: none of them are freed inside the loop, they accumulate on
frags_q, and there is no cond_resched() in the loop body.  A single
sendmsg() can therefore consume all allocatable memory, and on
CONFIG_PREEMPT_NONE it occupies the CPU long enough to trip the
softlockup watchdog:

  watchdog: BUG: soft lockup - CPU#3 stuck for 26s! [kworker/3:1:57]
  Workqueue: events rawsock_tx_work [nfc]
  Call Trace:
   nci_send_data+0x1ca/0x6b0 [nci]
   nci_transceive+0xbb/0x170 [nci]
   rawsock_tx_work+0xb5/0x1a0 [nfc]

max_pkt_payload_len comes straight from controller-supplied fields,
with no check for zero:

  ntf.c: conn_info->max_pkt_payload_len = ntf.max_data_pkt_payload_size;
  rsp.c: conn_info->max_pkt_payload_len = rsp->max_ctrl_pkt_payload_len;

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.

No legitimate configuration is affected: where the NCI spec does
mandate a zero Max Data Packet Payload Size -- the NFCEE Direct RF
Interface -- nci_rf_intf_activated_ntf_packet() takes the "goto
listen" shortcut, bypassing the assignment entirely.

Snapshot the field once with READ_ONCE() and use the snapshot for
both the check and the min_t() bound: the rx workqueue updates the
field without any lock held against this path, so without the
snapshot the check could validate a value the loop no longer
consumes.

nci_hci_send_data() also loops over the same field, but on a
different conn_info instance (ndev->hci_dev->conn_info) created by
nci_core_conn_create_rsp_packet(); a zero or one there underflows
the loop arithmetic and will be addressed in a separate patch.  This
guards the path carrying the reported bug.

Fixes: 6a2968aaf50c ("NFC: basic NCI protocol implementation")
Cc: stable@vger.kernel.org
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>

---

v1 claimed that nci_queue_tx_data_frags() is "the only place that
loops" over max_pkt_payload_len.  That holds for the RF data path
(ndev->rf_conn_info) but was overstated as a blanket claim: the
Sashiko review of v1 pointed out that nci_hci_send_data() loops over
the same field as well, albeit on a separate conn_info instance
(ndev->hci_dev->conn_info) -- pre-existing and unchanged here; it
will be fixed separately.

The READ_ONCE() snapshot follows the same review: with the check and
the loop reading the field independently, a store from the rx
workqueue in between would let the loop spin on a value the check
had just rejected.
---
 net/nfc/nci/data.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
index 4253edea5..eeb5260c5 100644
--- a/net/nfc/nci/data.c
+++ b/net/nfc/nci/data.c
@@ -104,6 +104,7 @@ static int nci_queue_tx_data_frags(struct nci_dev *ndev,
 	struct sk_buff_head frags_q;
 	struct sk_buff *skb_frag;
 	int frag_len;
+	u8 max_len;
 	int rc = 0;
 
 	pr_debug("conn_id 0x%x, total_len %d\n", conn_id, total_len);
@@ -114,11 +115,18 @@ static int nci_queue_tx_data_frags(struct nci_dev *ndev,
 		goto exit;
 	}
 
+	/* the rx workqueue may update the field concurrently */
+	max_len = READ_ONCE(conn_info->max_pkt_payload_len);
+
+	if (!max_len) {
+		rc = -EPROTO;
+		goto exit;
+	}
+
 	__skb_queue_head_init(&frags_q);
 
 	while (total_len) {
-		frag_len =
-			min_t(int, total_len, conn_info->max_pkt_payload_len);
+		frag_len = min_t(int, total_len, max_len);
 
 		skb_frag = nci_skb_alloc(ndev,
 					 (NCI_DATA_HDR_SIZE + frag_len),
-- 
2.50.1