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