[PATCH net 0/3] nfc: fix skb leaks in LLC nop and in data exchange error paths

[PATCH net 0/3] nfc: fix skb leaks in LLC nop and in data exchange error paths

[Joas Antonio dos Santos]

Three skb leaks in the NFC core, found by LLM-assisted code review while
auditing the HCI drivers that need real hardware.

Patch 1: with the LLC "nop" engine (used by the MEI attached pn544 and
microread) every frame sent to the controller is leaked.  Confirmed with
a test-only module that registers st21nfca with LLC nop on a virtual
phy and a userspace controller emulator: 20 device up/down cycles leak
598 skbuff_head_cache objects without the patch and none with it.

Patches 2 and 3: the HCI core and the HCI drivers (pn544, st21nfca,
microread) hand the response skb to the data exchange callback together
with an error when the controller reports a failed exchange, and NCI
does the same for a frame interface status error, but the raw socket
and LLCP callbacks only free the skb on success.  These two are compile
tested only (W=1); the leak follows from the code: __nfc_hci_cmd_completion()
gives the skb to the callback and never frees it itself, and
nci_add_rx_data_frag() passes it on with the error.


Joas Antonio dos Santos (3):
  nfc: hci: free the skb sent through LLC nop
  nfc: rawsock: free the response skb when the data exchange fails
  nfc: llcp: free the response skb when the data exchange fails

 net/nfc/hci/llc_nop.c | 11 ++++++++++-
 net/nfc/llcp_core.c   |  2 ++
 net/nfc/rawsock.c     |  4 ++--
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH net 3/3] nfc: llcp: free the response skb when the data exchange fails

[Joas Antonio dos Santos]

nfc_llcp_recv() is the data exchange callback for LLCP PDUs sent in
initiator mode, and it returns without freeing the skb when err is set.
Like for raw sockets, the skb can carry the response even when err is
set: the HCI core passes it on with the error derived from the
controller's result code, and NCI does the same for a frame interface
data packet with a non-zero status byte.  So each failed exchange leaks
an skb.

This was found by LLM-assisted code review.

Free the skb on the error path; it is NULL when the error does not come
with a response, and kfree_skb() handles that.

Fixes: d646960f7986 ("NFC: Initial LLCP support")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 net/nfc/llcp_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 74bf81700..f0632a0b9 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -1638,6 +1638,8 @@ void nfc_llcp_recv(void *data, struct sk_buff *skb, int err)
 
 	if (err < 0) {
 		pr_err("LLCP PDU receive err %d\n", err);
+		/* skb may carry the response even when the exchange failed */
+		kfree_skb(skb);
 		return;
 	}
 
-- 
2.43.0

Re: [PATCH net 3/3] nfc: llcp: free the response skb when the data exchange fails

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Double free of the LLCP response skb on digital (NFC-DEP)
  devices.
- [Critical] Double free of the raw-socket response skb on digital NFC-
  DEP targets, caused by the companion commit "nfc: rawsock: free the…
- [Critical] nfc: rawsock: double free of skb in
  rawsock_data_exchange_complete()

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but on ST21NFCA
  the failed HCI LLCP initiator exchange that the commit message…

--

nfc: llcp: free the response skb when the data exchange fails

This patch makes nfc_llcp_recv() free the skb it gets when the data
exchange reports an error. The commit message says HCI and NCI pass a
response skb along with the error, so the old early return leaked it. It
also says the skb is NULL when the error comes without a response.

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index 74bf817007cf2..f0632a0b94e74 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -1638,6 +1638,8 @@ void nfc_llcp_recv(void *data, struct sk_buff *skb, int err)
>  
>  	if (err < 0) {
>  		pr_err("LLCP PDU receive err %d\n", err);
> +		/* skb may carry the response even when the exchange failed */
> +		kfree_skb(skb);

[Severity: Critical]
The double free isn't in this hunk. The commit message builds on the
raw socket change earlier in this series ("Like for raw sockets"), and
that change looks like it can free the response skb twice on digital
stack devices.

With the rawsock patch applied, rawsock_data_exchange_complete() frees
the skb whenever err is set:

net/nfc/rawsock.c:rawsock_data_exchange_complete() {
    ...
	/* skb may carry the response even when the exchange failed */
	if (err)
		goto error_skb;
    ...
error_skb:
	kfree_skb(skb);
    ...
}

A raw socket can connect with nfc_protocol = NFC_PROTO_NFC_DEP.
rawsock_connect() calls nfc_activate_target(), and
digital_activate_target() stores that value in ddev->curr_protocol.
After that, every send from rawsock_tx_work() takes this path:

rawsock_tx_work()
  nfc_data_exchange(..., rawsock_data_exchange_complete, sk)
    digital_in_send()
      digital_in_send_dep_req()
        ... digital_in_recv_dep_res()

digital_in_recv_dep_res() has several protocol error paths that set
rc < 0 and jump to exit: while resp is still live:

  - a bad SoD length byte
  - an oversized frame
  - the NAD bit is set
  - a NACK is received
  - an ACK has a PNI mismatch
  - an ACK arrives with no chaining skb

On each of them it calls the callback and then frees resp itself:

net/nfc/digital_dep.c:digital_in_recv_dep_res() {
    ...
exit:
	data_exch->cb(data_exch->cb_context, resp, rc);

error:
	kfree(data_exch);
    ...
	if (rc)
		kfree_skb(resp);
}

So rawsock_data_exchange_complete() frees resp once, and the end of
digital_in_recv_dep_res() frees it again. The remote NFC-DEP peer
controls all of these error conditions. A single malformed DEP_RES is
enough to corrupt memory or crash the kernel.

Before this series, rawsock_data_exchange_complete() did not free the
skb on error. That makes this a regression, and both patches are
tagged Cc: stable.

The rawsock commit message says the skb is NULL when the error comes
without a response, and it lists digital as one of those cases. That
doesn't match digital_in_recv_dep_res(). It passes a live skb to the
callback but keeps ownership of it.

Which side is meant to own the skb when err is set? One option is for
digital_in_recv_dep_res() to pass NULL to the callback on error, as
digital_in_send_complete() already does. Then HCI, NCI and digital
would all follow one rule. Could that change go into this series ahead
of the rawsock and llcp patches? Could the data_exchange_cb_t
kernel-doc also state the ownership rule?

                 ^^^^

[Severity: Critical]
Can this free the response skb twice on digital stack devices (port100,
trf7970a, st95hf, nfcsim)?

On those devices LLCP runs over NFC-DEP. digital_dep_link_up() sets
ddev->curr_protocol = NFC_PROTO_NFC_DEP, so every LLCP exchange goes
through:

nfc_llcp_tx_work() / nfc_llcp_send_symm()
  nfc_data_exchange(..., nfc_llcp_recv, local)
    digital_in_send()
      digital_in_send_dep_req()

The exchange then completes in digital_in_recv_dep_res(). That function
still frees resp after it has called the callback:

net/nfc/digital_dep.c:digital_in_recv_dep_res() {
    ...
	rc = digital_skb_pull_dep_sod(ddev, resp);
	if (rc) {
		PROTOCOL_ERR("14.4.1.2");
		goto exit;
	}
    ...
exit:
	data_exch->cb(data_exch->cb_context, resp, rc);

error:
	kfree(data_exch);
    ...
	if (rc)
		kfree_skb(resp);
    ...
}

With this patch, nfc_llcp_recv() frees resp inside data_exch->cb(). The
"if (rc) kfree_skb(resp)" at the end then frees the same skb again.

Several other cases also reach exit: with a live resp and rc < 0:

  - resp->len > ddev->local_payload_max (-EMSGSIZE)
  - the NAD bit is set
  - a NACK is received
  - an ACK has a PNI mismatch
  - an ACK arrives with no ddev->chaining_skb

The remote NFC-DEP peer controls all of these. The SoD length byte is
checked before the CRC.

The commit message says:

    Free the skb on the error path; it is NULL when the error does not come
    with a response, and kfree_skb() handles that.

That doesn't seem to hold for digital NFC-DEP. There the error does come
with a response, but the producer keeps ownership of it.

The producers also disagree with each other:

  - digital_in_send_complete() frees resp and passes NULL on error.
  - HCI (__nfc_hci_cmd_completion()) and NCI hand ownership to the
    callback.
  - digital_in_recv_dep_res() frees resp itself after the callback.

The data_exchange_cb_t kernel-doc in include/net/nfc/nfc.h doesn't say
who owns the skb when err is set.

Could the producers be made to agree in the same series? For example,
digital_in_recv_dep_res() could pass NULL on error the way
digital_in_send_complete() does, or stop freeing resp after it calls the
callback. This patch is also tagged Cc: stable.

[Severity: Medium]
This isn't a bug introduced by this patch, but does a failed HCI LLCP
initiator exchange like the one in the commit message still leak the
response skb on st21nfca?

Take a READER_F target with supported_protocols == NFC_PROTO_NFC_DEP_MASK.
For it, st21nfca_hci_im_transceive() drops the callback it was given:

drivers/nfc/st21nfca/core.c:st21nfca_hci_im_transceive() {
    ...
	case ST21NFCA_RF_READER_F_GATE:
		if (target->supported_protocols == NFC_PROTO_NFC_DEP_MASK)
			return st21nfca_im_send_dep_req(hdev, skb);
    ...
}

So nfc_llcp_recv() is never reached on this path. The completion goes to
st21nfca_im_recv_dep_res_cb() instead.

When the controller returns an error result, __nfc_hci_cmd_completion()
passes both skb and err to that callback and does not free the skb. The
callback then returns early:

drivers/nfc/st21nfca/dep.c:st21nfca_im_recv_dep_res_cb() {
    ...
	if (err != 0)
		return;
    ...
}

Does this leak one skb per failed WR_XCHG_DATA exchange?

[Severity: Critical]
The commit message builds on the raw socket change ("Like for raw
sockets"). Is the same double free also reachable through the companion
commit f50c413847da ("nfc: rawsock: free the response skb when the data
exchange fails") in this series?

After that commit, rawsock_data_exchange_complete() frees the skb on any
error:

net/nfc/rawsock.c:rawsock_data_exchange_complete() {
    ...
	/* skb may carry the response even when the exchange failed */
	if (err)
		goto error_skb;
    ...
error_skb:
	kfree_skb(skb);
    ...
}

A SOCK_SEQPACKET raw socket does not need CAP_NET_RAW, and it can connect
with nfc_protocol = NFC_PROTO_NFC_DEP. digital_activate_target() then just
stores that value:

	ddev->curr_protocol = protocol;

>From then on, raw socket exchanges follow this path:

rawsock_tx_work()
  nfc_data_exchange()
    digital_in_send()
      digital_in_send_dep_req()
        ... digital_in_recv_dep_res()

A malformed DEP_RES (bad SoD length byte, NAD bit set, NACK, PNI
mismatch) reaches the exit: label with a live resp.
rawsock_data_exchange_complete() frees it, and then
"if (rc) kfree_skb(resp)" in digital_in_recv_dep_res() frees it again.

That commit message also says the skb:

    is NULL when the error does not
    come with a response (digital, timeouts, device close)

This doesn't seem to match digital NFC-DEP. In the baseline,
rawsock_data_exchange_complete() did not free the skb on error, so this
double free would be new with the series.

Should the fix for digital DEP ownership cover the raw socket path too?

>  		return;
>  	}
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179335864271.64596.805818095553030218%40gmail.com

[PATCH net 2/3] nfc: rawsock: free the response skb when the data exchange fails

[Joas Antonio dos Santos]

rawsock_data_exchange_complete() only frees the skb it is given when
the exchange succeeded.  But the skb can carry the response even when
err is set: the HCI core passes it on with the error derived from the
controller's result code (__nfc_hci_cmd_completion() gives it to the
callback and does not free it, and hci_transceive_cb() and the pn544,
st21nfca and microread data exchange callbacks forward it as is), and
NCI does the same for a frame interface data packet with a non-zero
status byte.  So each failed exchange on a raw socket leaks an skb, and
a controller or tag that keeps failing exchanges leaks memory without
bound.

This was found by LLM-assisted code review.

Free the skb on the error path too; it is NULL when the error does not
come with a response (digital, timeouts, device close), and
kfree_skb() handles that.

Fixes: 23b7869c0fd0 ("NFC: add the NFC socket raw protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 net/nfc/rawsock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c
index f7d7a599f..74ef3906c 100644
--- a/net/nfc/rawsock.c
+++ b/net/nfc/rawsock.c
@@ -156,8 +156,9 @@ static void rawsock_data_exchange_complete(void *context, struct sk_buff *skb,
 
 	pr_debug("sk=%p err=%d\n", sk, err);
 
+	/* skb may carry the response even when the exchange failed */
 	if (err)
-		goto error;
+		goto error_skb;
 
 	err = rawsock_add_header(skb);
 	if (err)
@@ -180,7 +181,6 @@ static void rawsock_data_exchange_complete(void *context, struct sk_buff *skb,
 error_skb:
 	kfree_skb(skb);
 
-error:
 	rawsock_report_error(sk, err);
 	sock_put(sk);
 }
-- 
2.43.0

Re: [PATCH net 2/3] nfc: rawsock: free the response skb when the data exchange fails

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Double free of the NFC-DEP response skb in
  rawsock_data_exchange_complete() (net/nfc/rawsock.c), added by this…

--

nfc: rawsock: free the response skb when the data exchange fails

This patch makes rawsock_data_exchange_complete() free the response skb
on the error path as well as on success. According to the commit message,
HCI and NCI pass a response skb to the callback together with a non-zero
err, so that skb leaks on every failed exchange.

> diff --git a/net/nfc/rawsock.c b/net/nfc/rawsock.c
> index f7d7a599fade7..74ef3906c2eda 100644
> --- a/net/nfc/rawsock.c
> +++ b/net/nfc/rawsock.c
> @@ -156,8 +156,9 @@ static void rawsock_data_exchange_complete(void *context, struct sk_buff *skb,
>  
>  	pr_debug("sk=%p err=%d\n", sk, err);
>  
> +	/* skb may carry the response even when the exchange failed */
>  	if (err)
> -		goto error;
> +		goto error_skb;

[Severity: Critical]
Can this double free the NFC-DEP response skb on the digital stack?

The commit message says:

  Free the skb on the error path too; it is NULL when the error does not
  come with a response (digital, timeouts, device close)

That is true for digital_in_send_complete() in net/nfc/digital_core.c,
which frees the skb and passes NULL. It is not true for
digital_in_recv_dep_res() in net/nfc/digital_dep.c. Several of its error
branches jump to exit while resp is still a live skb, for example:

	if (DIGITAL_NFC_DEP_NAD_BIT_SET(pfb)) {
		rc = -EIO;
		goto exit;
	}

The other such branches are the digital_skb_pull_dep_sod() failure, the
-EMSGSIZE check on resp->len, a NACK PDU, an ACK PNI mismatch, and an ACK
with no chaining_skb.

At exit, the function calls the callback and then frees resp itself:

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

error:
	kfree(data_exch);
	...
	if (rc)
		kfree_skb(resp);

With this change, rawsock_data_exchange_complete() frees resp first.
digital_in_recv_dep_res() then frees it a second time.

A raw socket can reach this path. rawsock_connect() passes
NFC_PROTO_NFC_DEP through nfc_activate_target() to
digital_activate_target(), which sets ddev->curr_protocol. After that,
digital_in_send() sends the exchange through digital_in_send_dep_req(),
with rawsock_data_exchange_complete() as the callback. The remote peer
controls the PFB bits and length of the DEP_RES frame, so it can trigger
any of these error branches.

The data_exchange_cb_t kernel-doc in include/net/nfc/nfc.h doesn't say
who owns @skb when @err is non-zero, and the producers handle it
differently:

  - HCI and NCI hand ownership to the callback.
  - digital_core frees the skb and passes NULL.
  - digital_dep passes the skb and frees it afterwards.

Would it make sense to fix the ownership rule in the producers as well?
For example, digital_in_recv_dep_res() could pass NULL on error, or stop
freeing resp after the callback. The rule could then be documented in
the kernel-doc.

The later patch in this series, "nfc: llcp: free the response skb when
the data exchange fails", only changes nfc_llcp_recv() and does not touch
digital_dep.c. So the double free is still there at the end of the
series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/179335864271.64596.805818095553030218%40gmail.com

[PATCH net 1/3] nfc: hci: free the skb sent through LLC nop

[Joas Antonio dos Santos]

nfc_hci_msg_tx_work() hands each frame to nfc_llc_xmit_from_hci() and
only frees it if that fails: on success the LLC engine owns it.  SHDLC
queues the frame and frees it once it is acked, but LLC nop passes it
straight to the driver's xmit, and the phy write operations of its
users (the MEI phy of pn544 and microread) copy the data out and leave
the skb alone, so every frame sent to the controller is leaked.

This was found by LLM-assisted code review and confirmed with a
test-only module that registers st21nfca with LLC nop on a virtual phy
and a userspace controller emulator: 20 device up/down cycles leaked
598 skbuff_head_cache objects, and none with this patch.

Free the frame in llc_nop_xmit_from_hci() once the driver has sent it.

Fixes: 8af00d48dc92 ("NFC: Add a nop (passthrough) llc module to llc core")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
---
 net/nfc/hci/llc_nop.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/nfc/hci/llc_nop.c b/net/nfc/hci/llc_nop.c
index f9c826a13..1603e435b 100644
--- a/net/nfc/hci/llc_nop.c
+++ b/net/nfc/hci/llc_nop.c
@@ -67,8 +67,17 @@ static void llc_nop_rcv_from_drv(struct nfc_llc *llc, struct sk_buff *skb)
 static int llc_nop_xmit_from_hci(struct nfc_llc *llc, struct sk_buff *skb)
 {
 	struct llc_nop *llc_nop = nfc_llc_get_data(llc);
+	int r;
 
-	return llc_nop->xmit_to_drv(llc_nop->hdev, skb);
+	/* The driver does not take ownership of the frame (the phy write
+	 * operations copy it out), and nfc_hci_msg_tx_work() only frees it
+	 * on error, so it is ours to free once it has been sent.
+	 */
+	r = llc_nop->xmit_to_drv(llc_nop->hdev, skb);
+	if (r == 0)
+		consume_skb(skb);
+
+	return r;
 }
 
 static const struct nfc_llc_ops llc_nop_ops = {
-- 
2.43.0