Re: [PATCH net] nfc: clear active_target when the target list is replaced

[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
---
nfc: clear active_target when the target list is replaced

This patch clears dev->active_target, stops the check_presence timer, and
resets RF and DEP link states in nfc_targets_found() if the currently
active target's array is being replaced. This prevents leaving a dangling
pointer when the target array is freed.

> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index a92a6566e6a0d..950807906645a 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -786,6 +786,21 @@ int nfc_targets_found(struct nfc_dev *dev,
>  
>  	dev->targets_generation++;
>  
> +	if (dev->active_target && dev->targets) {
> +		for (i = 0; i < dev->n_targets; i++) {
> +			if (dev->active_target != &dev->targets[i])
> +				continue;
> +
> +			if (dev->ops->check_presence)
> +				timer_delete_sync(&dev->check_pres_timer);
> +
> +			dev->active_target = NULL;
> +			dev->dep_link_up = false;
> +			dev->rf_mode = NFC_RF_NONE;
> +			break;

[Severity: High]
Could this cause a regression by bypassing the required driver and LLCP
teardown sequences?

By directly setting dev->dep_link_up = false, any subsequent user-space
call to nfc_dep_link_down() will return -EALREADY early. This permanently
skips nfc_llcp_mac_is_down(), which leaks LLCP sockets.

Also, it appears the hardware drivers will never receive the deactivate
signal since dev->ops->deactivate_target() and dev->ops->dep_link_down()
are bypassed. Could this lead to a hardware lockup or denial of service?

> +		}
> +	}
> +
>  	kfree(dev->targets);
>  	dev->targets = NULL;

[Severity: High]
This is a pre-existing issue, but I noticed that nfc_dep_link_is_up()
accesses and modifies core NFC state variables locklessly:

net/nfc/core.c:nfc_dep_link_is_up() {
	dev->dep_link_up = true;

	if (!dev->active_target && rf_mode == NFC_RF_INITIATOR) {
		...
		target = nfc_find_target(dev, target_idx);
	...
}

Drivers (like pn533) call nfc_dep_link_is_up() asynchronously without
holding device_lock(&dev->dev). Since nfc_targets_found() and
nfc_target_lost() can free and reallocate dev->targets while holding
the lock, could nfc_find_target() iterate over freed memory if it races
with them?

Furthermore, could this lockless access allow nfc_dep_link_is_up() to
assign a dangling pointer to dev->active_target right after it is
cleared by nfc_targets_found(), defeating the safety checks introduced
by this patch?