[PATCH net 0/2] nfc: digital: fix two bugs in NFC-DEP chaining and frame handling

[PATCH net 0/2] nfc: digital: fix two bugs in NFC-DEP chaining and frame handling

[Liu Chao]

Patch 1 fixes a guaranteed kernel panic (skb_under_panic) caused by
pushing protocol headers into a chaining_skb allocated with only
1 byte of headroom.  Both initiator and target paths are affected.
Switch to digital_skb_alloc() which reserves the same head and
tailroom used by every other send buffer in this file.

Patch 2 adds a length check in the ATR_RES completion callback
as hardening; the downstream handlers already validate, so the
practical impact is limited to misdirected dispatch on stale data.

Liu Chao (2):
  nfc: digital: reserve proper headroom for chaining_skb
  nfc: digital: check resp length in digital_tg_send_atr_res_complete()

 net/nfc/digital_dep.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)


base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
-- 
2.50.1

[PATCH net 1/2] nfc: digital: reserve proper headroom for chaining_skb

[Liu Chao]

digital_recv_dep_data_gather() allocates chaining_skb with
nfc_alloc_recv_skb(), which only reserves 1 byte of headroom.
Once the chained payload is fully gathered, it is passed to
digital_tg_send_dep_res() or digital_in_send_dep_req(), both of
which push a 3-byte DEP header plus up to 2 bytes of SoD, needing
at least 5 bytes of headroom.

skb_push() unconditionally calls skb_under_panic() when headroom is
insufficient, so this is a guaranteed kernel BUG.  A remote NFC peer
can trigger it in target mode by sending MI-flagged DEP_REQ fragments
followed by an ACK with matching PNI, or in initiator mode via the
symmetric chaining ACK path.  Normal DEP frames use digital_skb_alloc()
which gets headroom from ddev->tx_headroom, so the bug only manifests
through the chaining_skb path.

The skb_copy_expand() reallocation in the same function preserves the
original 1-byte headroom and does not add tailroom for the CRC or DID
byte appended by the send path.

Switch to digital_skb_alloc() which reserves the same headroom and
tailroom that every other send buffer in this file uses, and pass
ddev->tx_headroom and ddev->tx_tailroom to skb_copy_expand().

Fixes: c12715ab3f01 ("NFC: digital: Add NFC-DEP Receive Chaining Support")
Cc: stable@vger.kernel.org
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
 net/nfc/digital_dep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/nfc/digital_dep.c b/net/nfc/digital_dep.c
index 3982fa084..6d8e662a3 100644
--- a/net/nfc/digital_dep.c
+++ b/net/nfc/digital_dep.c
@@ -240,8 +240,8 @@ digital_recv_dep_data_gather(struct nfc_digital_dev *ddev, u8 pfb,
 
 	if (DIGITAL_NFC_DEP_MI_BIT_SET(pfb) && (!ddev->chaining_skb)) {
 		ddev->chaining_skb =
-			nfc_alloc_recv_skb(8 * ddev->local_payload_max,
-					   GFP_KERNEL);
+			digital_skb_alloc(ddev,
+					  8 * ddev->local_payload_max);
 		if (!ddev->chaining_skb) {
 			rc = -ENOMEM;
 			goto error;
@@ -251,9 +251,9 @@ digital_recv_dep_data_gather(struct nfc_digital_dev *ddev, u8 pfb,
 	if (ddev->chaining_skb) {
 		if (resp->len > skb_tailroom(ddev->chaining_skb)) {
 			new_skb = skb_copy_expand(ddev->chaining_skb,
-						  skb_headroom(
-							  ddev->chaining_skb),
-						  8 * ddev->local_payload_max,
+						  ddev->tx_headroom,
+						  8 * ddev->local_payload_max +
+							  ddev->tx_tailroom,
 						  GFP_KERNEL);
 			if (!new_skb) {
 				rc = -ENOMEM;
-- 
2.50.1

Re: [PATCH net 1/2] nfc: digital: reserve proper headroom for chaining_skb

[netdev-bot+sashiko]

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

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

- [High] State confusion between the RX reassembly buffer and the TX
  chaining buffer (`ddev->chaining_skb`) is left unfixed, and the patch…
- [High] The patch adds `ddev->tx_tailroom` to the gather buffer but does
  not protect that reserve from being consumed by gathered payload, so…
- [Medium] The commit message presents "Once the chained payload is fully
  gathered, it is passed to digital_tg_send_dep_res() or…

--

nfc: digital: reserve proper headroom for chaining_skb

The patch replaces the nfc_alloc_recv_skb() allocation of ddev->chaining_skb
in digital_recv_dep_data_gather() with digital_skb_alloc(), and passes
ddev->tx_headroom / ddev->tx_tailroom to the skb_copy_expand() call in the
same function, so the reassembly buffer carries the headroom and tailroom
the DEP send path expects.

A few questions about the change and the commit message below.

>     digital_recv_dep_data_gather() allocates chaining_skb with
>     nfc_alloc_recv_skb(), which only reserves 1 byte of headroom.
>     Once the chained payload is fully gathered, it is passed to
>     digital_tg_send_dep_res() or digital_in_send_dep_req(), both of
>     which push a 3-byte DEP header plus up to 2 bytes of SoD, needing
>     at least 5 bytes of headroom.

[Severity: Medium]
The message describes "Once the chained payload is fully gathered, it is
passed to digital_tg_send_dep_res() or digital_in_send_dep_req()" as the
intended design.  Is that transition actually intended?

ddev->chaining_skb serves two different purposes: the receive reassembly
buffer filled by digital_recv_dep_data_gather(), and the pending transmit
data set by digital_send_dep_data_prep().  The ACK branches consume whatever
is in the field as transmit data.

In target mode digital_tg_recv_dep_req() does:

	/* Keep on sending chained PDU */
	if (!ddev->chaining_skb ||
	    DIGITAL_NFC_DEP_PFB_PNI(pfb) !=
			ddev->curr_nfc_dep_pni) {
		rc = -EIO;
		goto exit;
	}
	...
	rc = digital_tg_send_dep_res(ddev, ddev->chaining_skb);

With ddev->tx_headroom = tx_headroom + DIGITAL_MAX_HEADER_LEN from
nfc_digital_allocate_device(), the skb_push() in digital_tg_send_dep_res()
no longer panics, so this branch now completes: the gathered payload is sent
back to the peer as a DEP_RES, nfc_tm_data_received() is never reached for
it, and ddev->curr_nfc_dep_pni advances on that transmission.

Could the message mention this behaviour change and the dual use of
chaining_skb?  As written, a reviewer or stable backporter would read it as
making the chaining path correct rather than removing one crash symptom.

> diff --git a/net/nfc/digital_dep.c b/net/nfc/digital_dep.c
> index 3982fa084737f..6d8e662a3819a 100644
> --- a/net/nfc/digital_dep.c
> +++ b/net/nfc/digital_dep.c
> @@ -240,8 +240,8 @@ digital_recv_dep_data_gather(struct nfc_digital_dev *ddev, u8 pfb,
>  
>  	if (DIGITAL_NFC_DEP_MI_BIT_SET(pfb) && (!ddev->chaining_skb)) {
>  		ddev->chaining_skb =
> -			nfc_alloc_recv_skb(8 * ddev->local_payload_max,
> -					   GFP_KERNEL);
> +			digital_skb_alloc(ddev,
> +					  8 * ddev->local_payload_max);

[Severity: High]
This makes the receive gather buffer send-capable.  Does that turn the
initiator-mode ACK path from an immediate skb_under_panic() into a NULL
pointer dereference?

digital_recv_dep_data_gather() leaves the reassembly buffer in
ddev->chaining_skb and returns NULL while the MI bit is set.  If the peer
then sends an ACK PDU with a matching PNI, digital_in_recv_dep_res() runs:

	rc = digital_in_send_dep_req(ddev, NULL,
				     ddev->chaining_skb,
				     ddev->data_exch);

ddev->data_exch is written only in digital_send_dep_data_prep():

		ddev->chaining_skb = skb;
		ddev->data_exch = data_exch;

and it is never reset to NULL.  ddev itself is zero allocated in
nfc_digital_allocate_device():

	ddev = kzalloc_obj(*ddev);

So if no transmit chaining ever happened on this link, is NULL passed down
as the completion argument, and does the next digital_in_recv_dep_res()
then dereference it at exit:

	data_exch->cb(data_exch->cb_context, resp, rc);

If a previous chained exchange did run, the error: label already did
kfree(data_exch), so can the same path be a use-after-free instead?

Separately, that ACK branch ends in goto free_resp without freeing the
valid per-exchange data_exch that came in as arg.  Does this leak the
digital_data_exch allocation?

Would separating the receive-gather state from the transmit-chaining state,
or rejecting an ACK PDU whose chaining_skb came from the gather path, be a
better fix than enlarging the buffer?

>  		if (!ddev->chaining_skb) {
>  			rc = -ENOMEM;
>  			goto error;
> @@ -251,9 +251,9 @@ digital_recv_dep_data_gather(struct nfc_digital_dev *ddev, u8 pfb,
>  	if (ddev->chaining_skb) {
>  		if (resp->len > skb_tailroom(ddev->chaining_skb)) {
>  			new_skb = skb_copy_expand(ddev->chaining_skb,
> -						  skb_headroom(
> -							  ddev->chaining_skb),
> -						  8 * ddev->local_payload_max,
> +						  ddev->tx_headroom,
> +						  8 * ddev->local_payload_max +
> +							  ddev->tx_tailroom,
>  						  GFP_KERNEL);

[Severity: High]
The tailroom reserve is added to the allocation, but is anything keeping the
gathered payload out of it?  The append guard is:

	if (resp->len > skb_tailroom(ddev->chaining_skb)) {

skb_tailroom() returns the whole end - tail region, which now includes the
ddev->tx_tailroom bytes this hunk reserves.  The fragment length is chosen
by the peer and only upper-bounded by the resp->len > ddev->local_payload_max
check in digital_tg_recv_dep_req(), so a peer sending MI fragments of one
payload byte each always satisfies 1 <= tailroom, and skb_put_data()
decrements the tailroom down to exactly 0 without ever taking the
skb_copy_expand() branch.

After that, an ACK PDU with a matching PNI reaches
digital_tg_send_dep_res(ddev, ddev->chaining_skb), which appends the DID
byte to the caller-supplied skb before digital_send_dep_data_prep() can copy
into a fresh buffer:

	if (ddev->did) {
		dep_res->pfb |= DIGITAL_NFC_DEP_PFB_DID_BIT;

		skb_put_data(skb, &ddev->did, sizeof(ddev->did));
	}

ddev->did is remote controlled (ddev->did = atr_req->did in
digital_tg_recv_atr_req(), values 1..14 accepted).  With tailroom at 0, does
this skb_put() run past skb->end into skb_over_panic() and BUG()?  The
gathered length is at least 8 * local_payload_max, which exceeds
remote_payload_max, so the copy into a fresh skb in
digital_send_dep_data_prep() always happens after the DID append.

Would comparing against skb_tailroom() minus ddev->tx_tailroom (plus the DID
byte), or bounding the total gathered length, keep the reserve intact?

One more accounting question on the same reserve.
nfc_digital_allocate_device() sets:

	ddev->tx_tailroom = tx_tailroom + DIGITAL_CRC_LEN;

That adds only the two CRC bytes on top of the driver request, while the
send path can append the DID byte plus CRC, i.e. three bytes.  Is the
reserve one byte short even when it is not consumed by payload?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912131852.1651462-1-liuc63%40xiaopeng.com

[PATCH net 2/2] nfc: digital: check resp length in digital_tg_send_atr_res_complete()

[Liu Chao]

digital_tg_send_atr_res_complete() reads resp->data[0] and
resp->data[offset] (offset 2 or 3) without checking resp->len.
The resp skb comes from the remote NFC peer, which controls its
length.  A short frame causes reads past the end of the received
data.

The downstream handlers (digital_tg_recv_psl_req, digital_tg_recv_dep_req)
each have their own length checks, so the consequence is a misdirected
dispatch on stale data rather than memory corruption.  Add the missing
check as hardening.

Fixes: 1c7a4c24fbfd ("NFC Digital: Add target NFC-DEP support")
Cc: stable@vger.kernel.org
Signed-off-by: Liu Chao <liuc63@xiaopeng.com>
---
 net/nfc/digital_dep.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/nfc/digital_dep.c b/net/nfc/digital_dep.c
index 6d8e662a3..5093b7b81 100644
--- a/net/nfc/digital_dep.c
+++ b/net/nfc/digital_dep.c
@@ -1467,16 +1467,19 @@ static void digital_tg_recv_psl_req(struct nfc_digital_dev *ddev, void *arg,
 static void digital_tg_send_atr_res_complete(struct nfc_digital_dev *ddev,
 					     void *arg, struct sk_buff *resp)
 {
-	int offset;
+	unsigned int offset;
 
 	if (IS_ERR(resp)) {
 		digital_poll_next_tech(ddev);
 		return;
 	}
 
-	offset = 2;
-	if (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB)
-		offset++;
+	if (!resp->len)
+		goto bad_frame;
+
+	offset = (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB) ? 3 : 2;
+	if (resp->len <= offset)
+		goto bad_frame;
 
 	ddev->atn_count = 0;
 
@@ -1484,6 +1487,12 @@ static void digital_tg_send_atr_res_complete(struct nfc_digital_dev *ddev,
 		digital_tg_recv_psl_req(ddev, arg, resp);
 	else
 		digital_tg_recv_dep_req(ddev, arg, resp);
+
+	return;
+
+bad_frame:
+	kfree_skb(resp);
+	digital_poll_next_tech(ddev);
 }
 
 static int digital_tg_send_atr_res(struct nfc_digital_dev *ddev,
-- 
2.50.1

Re: [PATCH net 2/2] nfc: digital: check resp length in digital_tg_send_atr_res_complete()

[netdev-bot+sashiko]

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

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

- [Medium] The commit message's justification ("The downstream handlers
  (digital_tg_recv_psl_req, digital_tg_recv_dep_req) each have their own…

Pre-existing issues:
- [Medium] Sibling instance of the same unchecked-length read remains in
  the same file: digital_in_recv_dep_res() reads resp->data[0] as the…
- [Medium] Frame-format disagreement kept (and re-written) by the patch:
  digital_tg_send_atr_res_complete() derives the SoD presence from the…

--

nfc: digital: check resp length in digital_tg_send_atr_res_complete()

This adds a resp->len check to digital_tg_send_atr_res_complete() before
reading resp->data[0] and resp->data[offset], and frees the skb and
restarts polling when the frame is too short.

A few questions below.

> nfc: digital: check resp length in digital_tg_send_atr_res_complete()
>
>     The downstream handlers (digital_tg_recv_psl_req, digital_tg_recv_dep_req)
>     each have their own length checks, so the consequence is a misdirected
>     dispatch on stale data rather than memory corruption.

[Severity: Medium]
Is this statement accurate for digital_tg_recv_dep_req()?  That handler
validates only resp->len >= sizeof(struct digital_dep_req_res), i.e. 3
bytes (dir, cmd, pfb), and then reads byte 3 before any further length
test:

net/nfc/digital_dep.c:digital_tg_recv_dep_req() {
	...
	size = sizeof(struct digital_dep_req_res);
	dep_req = (struct digital_dep_req_res *)resp->data;

	if (resp->len < size || dep_req->dir != DIGITAL_NFC_DEP_FRAME_DIR_OUT ||
	    dep_req->cmd != DIGITAL_CMD_DEP_REQ) {
		rc = -EIO;
		goto exit;
	}

	pfb = dep_req->pfb;

	if (DIGITAL_NFC_DEP_DID_BIT_SET(pfb)) {
		if (ddev->did && (ddev->did == resp->data[3])) {
	...
}

The "if (size > resp->len)" test that would cover byte 3 runs only after
that comparison, so it does not guard the read.

The precondition is a nonzero ddev->did, which the remote peer sets in
digital_tg_recv_atr_req():

	ddev->did = atr_req->did;

so a peer that negotiates a DID and then sends a 3-byte DEP_REQ payload
with the DID bit set in PFB makes digital_tg_recv_dep_req() read
resp->data[3] one byte past resp->len.  Should the check also be extended
there, or should the changelog drop the claim that the downstream handlers
cover this?

> diff --git a/net/nfc/digital_dep.c b/net/nfc/digital_dep.c
> index 6d8e662a3819a..5093b7b817202 100644
> --- a/net/nfc/digital_dep.c
> +++ b/net/nfc/digital_dep.c
> @@ -1467,16 +1467,19 @@ static void digital_tg_recv_psl_req(struct nfc_digital_dev *ddev, void *arg,
>  static void digital_tg_send_atr_res_complete(struct nfc_digital_dev *ddev,
>  					     void *arg, struct sk_buff *resp)
>  {
> -	int offset;
> +	unsigned int offset;
>  
>  	if (IS_ERR(resp)) {
>  		digital_poll_next_tech(ddev);
>  		return;
>  	}
>  
> -	offset = 2;
> -	if (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB)
> -		offset++;
> +	if (!resp->len)
> +		goto bad_frame;
> +
> +	offset = (resp->data[0] == DIGITAL_NFC_DEP_NFCA_SOD_SB) ? 3 : 2;
> +	if (resp->len <= offset)
> +		goto bad_frame;

[Severity: Medium]
This isn't a bug introduced by this patch, since the byte-value heuristic
is only reformatted into a ternary here, but while this offset computation
is being rewritten, would it be better to derive the SoD presence from
ddev->curr_rf_tech the way the encoder/decoder pair does?

net/nfc/digital_dep.c:digital_skb_push_dep_sod() {
	if (ddev->curr_rf_tech == NFC_DIGITAL_RF_TECH_106A)
		*(u8 *)skb_push(skb, sizeof(u8)) = DIGITAL_NFC_DEP_NFCA_SOD_SB;
}

net/nfc/digital_dep.c:digital_skb_pull_dep_sod() {
	if (ddev->curr_rf_tech == NFC_DIGITAL_RF_TECH_106A)
		skb_pull(skb, sizeof(u8));
}

In 212F/424F target mode there is no SB byte, so data[0] is the LEN byte.
Can a LEN of 0xF0 (240, legal since local_payload_max is
DIGITAL_PAYLOAD_SIZE_MAX) make this pick offset 3 and read the PFB byte as
the command byte?

DIGITAL_CMD_PSL_REQ is 0x04 and DIGITAL_NFC_DEP_PFB_DID_BIT is also 0x04,
so a first DEP_REQ (I-PDU, PNI 0, DID in use) of exactly that size would
have PFB == 0x04 and be dispatched to digital_tg_recv_psl_req(), which
then drops it:

net/nfc/digital_dep.c:digital_tg_recv_psl_req() {
	if (resp->len != sizeof(struct digital_psl_req) ||
	...
		rc = -EIO;
		goto exit;
	}
}

Does that stall the DEP session, since the frame is freed with no reply?
The new bounds check is computed from the same offset, so it inherits the
disagreement.

>  
>  	ddev->atn_count = 0;
>  
> @@ -1484,6 +1487,12 @@ static void digital_tg_recv_psl_req(struct nfc_digital_dev *ddev, void *arg,
>  		digital_tg_recv_psl_req(ddev, arg, resp);
>  	else
>  		digital_tg_recv_dep_req(ddev, arg, resp);
> +
> +	return;
> +
> +bad_frame:
> +	kfree_skb(resp);
> +	digital_poll_next_tech(ddev);
>  }

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but there
is a sibling unchecked read of the same kind in the same file, in
digital_in_recv_dep_res().  Would it make sense to cover it in the same
series?

net/nfc/digital_dep.c:digital_in_recv_dep_res() {
	...
	if (size > resp->len) {
		rc = -EIO;
		goto error;
	}

	skb_pull(resp, size);
	...
	case DIGITAL_NFC_DEP_PFB_SUPERVISOR_PDU:
	...
		rtox = DIGITAL_NFC_DEP_RTOX_VALUE(resp->data[0]);
}

With a 3-byte DEP_RES payload (dir, cmd, pfb and no DID/NAD bit), size is
3, so both "resp->len < size" and "size > resp->len" pass, and the
skb_pull() leaves resp->len == 0.  The SUPERVISOR_PDU plus timeout-bit
branch then reads resp->data[0] one byte past resp->len and feeds the
masked value to digital_in_send_rtox() as a timeout.  Both ddev->atn_count
and ddev->nack_count are zeroed just above, so the branch is reachable.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260912131852.1651462-1-liuc63%40xiaopeng.com