Re: [PATCH net 2/5] nfc: nci: hci: consume the HCP response exactly once

[netdev-bot+sashiko@kernel.org]

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

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

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] nci_hci_take_resp() is supposed to return -EPROTO "when the
  request was completed without a response" and not "reuse whatever skb…
- [Low] nci_hci_take_resp() detaches the response with a plain load
  followed by a plain store (`rx_skb = conn_info->rx_skb;…

--

nfc: nci: hci: consume the HCP response exactly once

This patch adds nci_hci_take_resp(). The helper detaches the HCP response
stored in conn_info->rx_skb, parses it, and either hands it to the caller
or frees it. It returns -EPROTO when a request completes without a stored
response. nci_hci_send_cmd(), nci_hci_set_param(), nci_hci_get_param() and
now nci_hci_open_pipe() all use it, which closes the use-after-free and
NULL dereference shown in the commit message.

> diff --git a/net/nfc/nci/hci.c b/net/nfc/nci/hci.c
> index c03e8a0bd3bd6..bdfe9eee6358c 100644
> --- a/net/nfc/nci/hci.c
> +++ b/net/nfc/nci/hci.c
> @@ -107,6 +107,38 @@ static int nci_hci_result_to_errno(u8 result)
>  	}
>  }
>  
> +/* Take the HCP response stored by nci_hci_resp_received().
> + *
> + * The response skb is detached from conn_info so that it is consumed
> + * exactly once.  A request can also be completed without a response
> + * being stored (an HCP command or event received while the request is
> + * pending, or an unrelated NCI response), so a missing response is an
> + * error rather than a reason to reuse whatever skb was stored last.
> + */
> +static int nci_hci_take_resp(struct nci_conn_info *conn_info,
> +			     struct sk_buff **skb)
> +{
> +	const struct nci_hcp_message *message;
> +	struct sk_buff *rx_skb;
> +	int r;
> +
> +	rx_skb = conn_info->rx_skb;
> +	conn_info->rx_skb = NULL;

[Severity: Low]
Can this read-and-clear race with nci_hci_resp_received()? Every caller
runs nci_hci_take_resp() after nci_request() has dropped ndev->req_lock.
The producer runs from rx_wq and shares no lock with this code.

Suppose an HCI command is completed early by an HCP event or an OK-status
NCI notification, and its real response comes in during this window:

caller                                rx_wq
nci_hci_take_resp()
  rx_skb = conn_info->rx_skb; /* NULL */
                                      nci_hci_resp_received()
                                        conn_info->rx_skb = skb;
  conn_info->rx_skb = NULL;

The skb stored by rx_wq would be overwritten and leaked, and the call
would return -EPROTO. These are also plain, unannotated concurrent
accesses that KCSAN would report.

Two consumers detaching the same skb doesn't look reachable in-tree. The
nfc generic netlink family doesn't set parallel_ops, so genl_mutex
serializes the callers.

Would xchg(), or detaching under a lock shared with the producer, avoid
the lost store?

> +	if (!rx_skb)
> +		return -EPROTO;

[Severity: Medium]
Can a request still get an earlier request's response here? The comment
above says a missing response is an error rather than a reason to reuse
whatever skb was stored last. However, the only check is for NULL.

Several paths call nci_req_complete(ndev, NCI_STATUS_OK) whenever any
request is pending:

  nci_hci_hcp_message_rx() for HCP events and commands (run later from
  msg_rx_work)
  nci_rf_deactivate_ntf_packet(), nci_nfcee_discover_ntf_packet() and
  nci_core_reset_ntf_packet() in ntf.c
  the zero-status NCI response handlers in rsp.c

If one of these completes HCI command R1 early, this correctly returns
-EPROTO. When R1's real response arrives afterwards,
nci_hci_resp_received() still stores it unconditionally:

net/nfc/nci/hci.c:nci_hci_resp_received() {
	...
	conn_info->rx_skb = skb;

exit:
	nci_req_complete(ndev, NCI_STATUS_OK);
}

That function doesn't check whether a request is pending or which pipe
the response is for. It also doesn't free an skb that is already stored.
The same thing happens when R1 hits NCI_DATA_TIMEOUT in __nci_request()
and the NFCC answers late.

None of nci_hci_send_cmd(), nci_hci_get_param(), nci_hci_set_param() or
nci_hci_open_pipe() clears rx_skb before calling nci_request().

For the next command R2, there seem to be two outcomes.

If R2's own response arrives, it overwrites the orphan and the orphaned
skb leaks. Could an NFCC that keeps sending late or unsolicited responses
leak skbs without bound this way?

If R2 is also completed early, nci_hci_take_resp() parses R1's HCP result
code and hands R1's payload to R2's caller. R2's own response then becomes
the next orphan, so the mismatch carries forward. Callers that would get
the wrong payload include:

  nci_hci_create_pipe(), which reads resp->pipe at offset 4 with no
  length check, so a payload-less ANY_OPEN_PIPE response would be read
  past its data
  nci_hci_dev_session_init() (session identity)
  st_nci_hci_load_session() (pipe list)
  st_nci_control_se() (host list)
  st_nci_se_get_atr() (ATR)

The commit message says the helper returns -EPROTO "when the request was
completed without a response". In this sequence the request succeeds with
the previous request's response.

Would it make sense to drop any stale rx_skb before sending, for example
in the req callback under req_lock? Separately, nci_hci_resp_received()
could store a response only while a request is pending, freeing any
previous skb.

> +
> +	message = (struct nci_hcp_message *)rx_skb->data;
> +	r = nci_hci_result_to_errno(NCI_HCP_MSG_GET_CMD(message->header));
> +	skb_pull(rx_skb, NCI_HCI_HCP_MESSAGE_HEADER_LEN);
> +
> +	if (!r && skb)
> +		*skb = rx_skb;
> +	else
> +		kfree_skb(rx_skb);
> +
> +	return r;
> +}

[ ... ]

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