Re: [PATCH net 1/5] nfc: nci: don't drop data received on HCI and loopback connections

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 2 (3) · 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).

- [Medium] Three completion paths pass ndev->cur_conn_id where they
  should pass the RF connection id, and after this patch that can leave…
- [Medium] After this patch, completions on non-RF connections still tear
  down the RF exchange's timeout state but no longer release…

Pre-existing issues:
- [High] HCI work can use the device after it is freed during teardown.
- [High] At the target commit, this patch lets NFCC data on the HCI
  connection reach nci_hci_data_received_cb() and nci_hci_msg_rx_work()…
- [Medium] nci_nfcc_loopback_cb() (net/nfc/nci/core.c:409-423) ignores
  err, stores the skb (possibly NULL) in conn_info->rx_skb, and…
- [Medium] This patch makes the conn_info->rx_skb lifecycle reachable
  again.
- [Medium] For an HCP RESPONSE, nci_hci_hcp_message_rx() completes the
  request twice.

--

nfc: nci: don't drop data received on HCI and loopback connections

This patch limits the NCI_DATA_EXCHANGE claim in
nci_data_exchange_complete() to data on the static RF connection. Data
on HCI over NCI and NFCC loopback connections goes to their
data_exchange_cb again, instead of being freed. That undoes the
regression from 8265a626cc14 where every HCI and loopback request timed
out.

> diff --git a/net/nfc/nci/data.c b/net/nfc/nci/data.c
> index 4253edea5c8db..76ec891a02b09 100644
> --- a/net/nfc/nci/data.c
> +++ b/net/nfc/nci/data.c
> @@ -46,8 +46,13 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
>  	timer_delete_sync(&ndev->data_timer);
>  	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
>  
> -	/* Claim completion atomically -- both close and rx_work may race here */
> -	if (!test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
> +	/* Claim completion atomically -- both close and rx_work may race here.
> +	 * NCI_DATA_EXCHANGE only tracks nci_transceive() on the static RF
> +	 * connection; data received on other logical connections (HCI over
> +	 * NCI, NFCC loopback) is always handed to their callback.
> +	 */
> +	if (conn_id == NCI_STATIC_RF_CONN_ID &&
> +	    !test_and_clear_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {

[Severity: Medium]
Can this leave NCI_DATA_EXCHANGE set with nothing left to clear it?

The callers gated on NCI_DATA_EXCHANGE pass ndev->cur_conn_id, not the
RF connection id. In nci_rx_work():

	if (test_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags)) {
		/* complete the data exchange transaction, if exists */
		if (test_bit(NCI_DATA_EXCHANGE, &ndev->flags))
			nci_data_exchange_complete(ndev, NULL,
						   ndev->cur_conn_id,
						   -ETIMEDOUT);

Both completions in nci_close_device() do the same thing with -ENODEV.

Only nci_transceive() sets NCI_DATA_EXCHANGE, so a set bit means an RF
exchange on conn 0 is pending. But every nci_send_data() overwrites
cur_conn_id without a lock. That includes HCI sends through
nci_hci_send_data() and nci_nfcc_loopback(), and nci_transceive()
doesn't take req_lock.

Say an RF exchange from rawsock or LLCP is pending. An HCI or loopback
send then sets cur_conn_id to a non-RF connection and re-arms data_timer
in nci_tx_work(). If the timer fires:

nci_rx_work()
  nci_data_exchange_complete(ndev, NULL, ndev->cur_conn_id, -ETIMEDOUT)
    timer_delete_sync(&ndev->data_timer);
    clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
    conn_id != NCI_STATIC_RF_CONN_ID, so test_and_clear_bit() is skipped
    HCI or loopback cb(cb_context, NULL, -ETIMEDOUT)

The RF callback never runs. NCI_DATA_EXCHANGE stays set and no timer is
left, so every later nci_transceive() returns -EBUSY. Only an
RF_DEACTIVATE_NTF or a close with NCI_UP set clears it. Before this
patch, test_and_clear_bit() released the bit whatever the conn_id was.

The same mismatch lets a close-time completion and an rx_work delivery
on the same non-RF connection both reach the callback. The atomic claim
from 8265a626cc14 no longer covers them, so nci_nfcc_loopback_cb() could
overwrite a just-stored rx_skb with NULL.

One part of this is older than this patch. On close the RF callback
never runs, which leaks the rawsock sock_hold(), but the old code also
called the cur_conn_id callback.

Should the paths gated on NCI_DATA_EXCHANGE pass NCI_STATIC_RF_CONN_ID
instead, as nci_rf_deactivate_ntf_packet() already does?

[Severity: Medium]
With the claim now limited to conn 0, a completion on a non-RF
connection still runs this on the device-wide data_timer:

	timer_delete_sync(&ndev->data_timer);
	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);

but it no longer releases NCI_DATA_EXCHANGE. Does this cancel the only
timeout of a pending RF nci_transceive()?

nci_tx_work() re-arms data_timer for frames on any connection. While an
RF exchange is pending, any HCI or loopback data on its own connection
deletes the RF exchange's timer. That includes unsolicited HCP events
from the NFCC or SE.

If data_timer has already fired and rx_work handles such a packet in
its dequeue loop, NCI_DATA_EXCHANGE_TO is cleared before the check at
the end of nci_rx_work(). The timeout is lost.

nci_core_conn_intf_error_ntf_packet() has the same shape:

	if (test_bit(NCI_DATA_EXCHANGE, &ndev->flags))
		nci_data_exchange_complete(ndev, NULL, ntf->conn_id, -EIO);

If the NFCC sends a non-RF ntf->conn_id, this deletes the RF timer and
leaves the bit set.

If the RF response is then lost, NCI_DATA_EXCHANGE stays set. Every
later nci_transceive() returns -EBUSY until an RF deactivation. Before
this patch, the non-RF completion's test_and_clear_bit() released the
bit.

The new comment says NCI_DATA_EXCHANGE only tracks the RF connection,
but the timer and TO state are still shared. Should the timer and TO
handling also be limited to conn_id == NCI_STATIC_RF_CONN_ID, or be
made per connection?

>  		kfree_skb(skb);
>  		return;
>  	}

[Severity: Medium]
This isn't a bug introduced by this patch, but now that non-RF data is
always forwarded again, can nci_nfcc_loopback_cb() report success with
a NULL skb?

	conn_info->rx_skb = skb;

	nci_req_complete(ndev, NCI_STATUS_OK);

The callback ignores err. A NULL skb can come from the rx_work timeout,
from CORE_INTERFACE_ERROR_NTF, or from the skb_cow_head() failure path
in nci_add_rx_data_frag() (err = -ENOMEM). In each case the request
completes with OK, and nci_nfcc_loopback() returns 0 with
*resp == NULL. st_nci_loopback() then dereferences it:

	r = nci_nfcc_loopback(ndev, data, data_len, &skb);
	if (r < 0)
		return r;

	msg = nfc_vendor_cmd_alloc_reply_skb(dev, ST_NCI_VENDOR_OUI,
					     LOOPBACK, skb->len);

With this patch, the -ENOMEM case is reachable without an RF exchange.

A later patch in this series, "nfc: nci: consume the loopback response
exactly once", fixes the NULL dereference: nci_nfcc_loopback() detaches
rx_skb and returns -EPROTO when it is NULL. One minor gap remains after
that: a non-NULL skb that arrives together with an error is still
reported as success.

[Severity: Medium]
This is a pre-existing issue, but this patch makes the
conn_info->rx_skb lifecycle reachable again. At this commit rx_skb is
never consumed in these cases, so those skbs leak:

  nci_hci_set_param()
  nci_hci_send_cmd(), when skb is NULL or the HCI result is non-zero
  nci_hci_open_pipe()

Also, any skb that is handed out leaves conn_info->rx_skb dangling.

Later patches in this series fix most of that. "nfc: nci: hci: consume
the HCP response exactly once" adds nci_hci_take_resp(), and "nfc: nci:
consume the loopback response exactly once" does the same for loopback.

One gap remains after those. nci_hci_resp_received() and
nci_nfcc_loopback_cb() both still do:

	conn_info->rx_skb = skb;

without freeing an unconsumed previous skb. Does each late reply after
nci_request() has timed out, or each extra unsolicited HCP RESPONSE from
the NFCC, leak one skb? Nothing frees rx_skb when the connection or
device is torn down either.

[Severity: High]
This isn't a bug introduced by this patch, but HCI delivery was
effectively unreachable after 8265a626cc14, and this patch turns it
back on. Is msg_rx_work ever synchronized with device teardown?

nci_hci_data_received_cb() queues non-RESPONSE messages on the system
workqueue:

		skb_queue_tail(&ndev->hci_dev->msg_rx_queue, hcp_skb);
		schedule_work(&ndev->hci_dev->msg_rx_work);

nci_unregister_device() only flushes and destroys the NCI cmd, rx and
tx workqueues. nci_free_device() calls nci_hci_deallocate():

void nci_hci_deallocate(struct nci_dev *ndev)
{
	kfree(ndev->hci_dev);
}

and then frees ndev. Nothing in net/nfc/nci calls cancel_work_sync() or
flush_work() on msg_rx_work. msg_rx_queue and rx_hcp_frags are never
purged.

In the NCI_UP branch, nci_close_device() keeps the transport open
during the reset request. HCI packets received then still go through
rx_work and schedule msg_rx_work. That work can sleep: for example,
st_nci_hci_connectivity_event_received() calls
devm_kzalloc(GFP_KERNEL) and nfc_se_transaction().

Suppose st_nci_remove()->nci_unregister_device()->nci_free_device()
runs meanwhile. Can nci_hci_msg_rx_work() then touch the freed hdev and
ndev, or call nci_hci_send_data()->queue_work() on the destroyed tx_wq?
Any queued fragments and messages would also leak.

[Severity: Medium]
This isn't a bug introduced by this patch, but now that HCI responses
reach their callback again, each HCP RESPONSE completes the request
twice:

nci_hci_hcp_message_rx()
  nci_hci_resp_received()
    nci_req_complete(ndev, NCI_STATUS_OK);
  nci_req_complete(ndev, NCI_STATUS_OK);

nci_req_complete() only checks req_status == NCI_REQ_PEND. Suppose
rx_work is preempted after the first complete(). The waiter can return,
drop req_lock and start the next nci_request(), which sets
NCI_REQ_PEND. Could the second call then complete that new request
early with OK?

req_lock serializes submitters but doesn't exclude the receive worker.
In the final series state, the next HCI request would get -EPROTO from
nci_hci_take_resp(). A plain NCI command would instead return success
before its RSP arrives, and its real response would later be stored and
never consumed.

At this commit, the race can also cause a use-after-free of a stale
rx_skb. "nfc: nci: hci: consume the HCP response exactly once" fixes
that part.

[Severity: High]
This is a pre-existing issue, but at this commit NFCC data on the HCI
connection reaches nci_hci_data_received_cb() and nci_hci_msg_rx_work()
again. nci_hci_hcp_message_rx() ends with:

	nci_req_complete(ndev, NCI_STATUS_OK);

It does this for COMMAND, EVENT and unknown types as well, and
conn_info->rx_skb is never reset to NULL.

Suppose an unsolicited HCP EVENT or COMMAND arrives while one of these
is waiting:

  nci_hci_send_cmd()
  nci_hci_set_param()
  nci_hci_get_param()
  nci_nfcc_loopback()

The request then completes with OK. Can the waiter skb_pull() a NULL or
already-freed conn_info->rx_skb and hand it out again? That would lead
to a double free in callers such as st_nci_hci_load_session() and
st_nci_loopback().

Later patches in this series fix this. "nfc: nci: hci: consume the HCP
response exactly once" adds nci_hci_take_resp(), which detaches rx_skb
and returns -EPROTO when it is NULL. "nfc: nci: consume the loopback
response exactly once" does the same for nci_nfcc_loopback().

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