Re: [PATCH net v2 1/1] net: nfc: fix use-after-free in nfc_get_local_general_bytes

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/7023ed1e8d7522917d699222d9d5db7135184abb.1788157546.git.rakukuip@gmail.com?part=1
---
net: nfc: fix use-after-free in nfc_get_local_general_bytes

This commit refactors nfc_get_local_general_bytes to accept a caller-provided
output buffer instead of returning a pointer to local->gb. All callers are
updated to provide their own destination buffers to prevent use-after-free
issues when the local reference count drops to zero.

[ ... ]

> diff --git a/net/nfc/nci/core.c b/net/nfc/nci/core.c
> --- a/net/nfc/nci/core.c
> +++ b/net/nfc/nci/core.c
> @@ -780,26 +780,24 @@ static int nci_set_local_general_bytes(struct nfc_dev *nfc_dev)
>  {
>  	struct nci_dev *ndev = nfc_get_drvdata(nfc_dev);
>  	struct nci_set_config_param param;
> +	u8 gb[NFC_MAX_GT_LEN];
>  	int rc;
>  
> -	param.val = nfc_get_local_general_bytes(nfc_dev, &param.len);
> -	if ((param.val == NULL) || (param.len == 0))
> +	nfc_get_local_general_bytes(nfc_dev, gb, sizeof(gb),
> +				    &param.len);
> +	if (param.len == 0)
>  		return 0;
>  
> -	if (param.len > NFC_MAX_GT_LEN)
> -		return -EINVAL;
> -
> +	param.val = gb;
>  	param.id = NCI_PN_ATR_REQ_GEN_BYTES;
>  
> -	rc = nci_request(ndev, nci_set_config_req, &param,
> -			 msecs_to_jiffies(NCI_SET_CONFIG_TIMEOUT));
> +	rc = nci_set_config(ndev, param.id, param.len, param.val);

[Severity: High]
Does replacing nci_request with nci_set_config introduce a locking regression
here?

The original code used nci_request, which acquires ndev->req_lock before
proceeding. The new helper nci_set_config directly calls the unlocked
__nci_request:

net/nfc/nci/core.c:nci_set_config() {
    ...
    return __nci_request(ndev, nci_set_config_req, &param,
                         msecs_to_jiffies(NCI_SET_CONFIG_TIMEOUT));
}

Because __nci_request modifies ndev->req_status and reinitializes
ndev->req_completion, calling it without synchronization can corrupt the
wait queue if another thread is concurrently executing a locked nci_request.

This polling path can be triggered by unprivileged user space via the netlink
NFC_CMD_START_POLL command. Is there a way to restore the req_lock
synchronization for these configuration updates?

>  	if (rc)
>  		return rc;
>  
>  	param.id = NCI_LN_ATR_RES_GEN_BYTES;
>  
> -	return nci_request(ndev, nci_set_config_req, &param,
> -			   msecs_to_jiffies(NCI_SET_CONFIG_TIMEOUT));
> +	return nci_set_config(ndev, param.id, param.len, param.val);
>  }