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

[Simon Horman <horms@kernel.org>]

On Wed, Sep 09, 2026 at 01:19:24PM +0800, Ren Wei wrote:
> From: Luxiao Xu <rakukuip@gmail.com>
> 
> Commit 6709d4b7bc2e ("net: nfc: Fix use-after-free caused by
> nfc_llcp_find_local") attempted to fix a use-after-free (UAF) issue by
> invoking nfc_llcp_local_put(local) after accessing local->gb. However,
> if the reference count drops to zero, local is freed immediately,
> leading to a use-after-free when callers access the returned pointer.
> Alternative approaches using dynamic allocation (e.g. kmemdup) introduced
> memory leaks because callers consistently treat the returned pointer as
> borrowed memory.
> 
> Fix this properly by refactoring nfc_llcp_general_bytes() and
> nfc_get_local_general_bytes() to accept a caller-provided output buffer
> (out_gb) and its maximum length (gb_max_len). The general bytes are
> safely copied into out_gb before calling nfc_llcp_local_put(local),
> ensuring safe lifetime management without ownership transfer complications.
> 
> Update all callers across drivers (microread, pn533, pn544, st21nfca,
> digital_dep, and nci) to provide their own destination buffers and pass
> them to nfc_get_local_general_bytes().
> 
> Fixes: 6709d4b7bc2e ("net: nfc: Fix use-after-free caused by nfc_llcp_find_local")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: LLM
> Signed-off-by: Luxiao Xu <rakukuip@gmail.com>
> Signed-off-by: Ren Wei <weir@nebusec.ai>
> ---
> v3:
>  - Include <net/nfc/nfc.h> in pn533.h to fix build error in uart.c
>    caused by undefined NFC_MAX_GT_LEN.
>  - Restore nci_request() in nci_set_local_general_bytes() to preserve
>    ndev->req_lock synchronization (avoid unlocked __nci_request() via
>    nci_set_config()).
> v2:
>  - Use caller-provided output buffers to fix UAF instead of dynamic
>    allocation (kmemdup), avoiding memory leaks.

Overall this patch looks good to me.
But I'd appreciate it if you could consider my suggestion further below.

Reviewed-by: Simon Horman <horms@kernel.org>

...

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index dc65c719f35f..24cf212bf3f8 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -635,23 +635,32 @@ static int nfc_llcp_build_gb(struct nfc_llcp_local *local)
>  	return ret;
>  }
>  
> -u8 *nfc_llcp_general_bytes(struct nfc_dev *dev, size_t *general_bytes_len)
> +u8 *nfc_llcp_general_bytes(struct nfc_dev *dev, u8 *out_gb, size_t gb_max_len,
> +			   size_t *general_bytes_len)
>  {
>  	struct nfc_llcp_local *local;
>  
> +	if (!out_gb || !general_bytes_len)
> +		return NULL;
> +
>  	local = nfc_llcp_find_local(dev);
> -	if (local == NULL) {
> +	if (!local) {
>  		*general_bytes_len = 0;
>  		return NULL;
>  	}
>  
>  	nfc_llcp_build_gb(local);
>  
> -	*general_bytes_len = local->gb_len;
> +	if (local->gb_len) {
> +		*general_bytes_len = min_t(size_t, local->gb_len, gb_max_len);
> +		memcpy(out_gb, local->gb, *general_bytes_len);
> +	} else {
> +		*general_bytes_len = 0;
> +	}
>  
>  	nfc_llcp_local_put(local);
>  
> -	return local->gb;
> +	return out_gb;
>  }

As far as I can see the return value of nfc_llcp_general_bytes() is now
ignored. And for a bug fix the approach you have taken looks good,
as the interface provided by nfc_llcp_general_bytes() is only slightly
modified.

But, FWIIW, I think it would be cleaner if nfc_llcp_general_bytes()
returned the length, or 0 on error. And the general_bytes_len parameter was
passed by value rather than reference.

Something like this:

size_t nfc_llcp_general_bytes(struct nfc_dev *dev, u8 *out_gb,
			      size_t gb_max_len, size_t general_bytes_len)
{
	...

	if (error_condition)
		return 0;

	...

	return general_bytes_len;
}

Perhaps that approach could be considered as a follow-up.