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

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

[Yinhao Hu]

nfc_activate_target() and nfc_dep_link_up() cache dev->active_target as a
raw pointer into the dev->targets array. When a later poll reports new
targets, nfc_targets_found() frees and replaces dev->targets but does not
clear dev->active_target, so the cached pointer is left dangling into
freed memory. Any subsequent NFC core path that dereferences
dev->active_target->idx then reads the freed memory, e.g.
nfc_deactivate_target(), nfc_data_exchange().

When nfc_targets_found() is about to free the current target array, clear
dev->active_target if it points into that array, and tear down the
associated active state (stop the presence-check timer, drop the DEP link
and reset the RF mode) as nfc_deactivate_target() does.

Fixes: 900994332675 ("NFC: Cache the core NFC active target pointer instead of its index")
Signed-off-by: Yinhao Hu <dddddd@hust.edu.cn>
---
 net/nfc/core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/nfc/core.c b/net/nfc/core.c
index a92a6566e6a0..950807906645 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;
+		}
+	}
+
 	kfree(dev->targets);
 	dev->targets = NULL;
 
-- 
2.43.0

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

[Simon Horman]

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?

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

[Simon Horman]

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://netdev-ai.bots.linux.dev/sashiko/
---
nfc: clear active_target when the target list is replaced

This patch updates nfc_targets_found() so that when the active target
points into the dev->targets array about to be freed, the cached pointer
is cleared and a partial teardown (presence-check timer stop, dep_link_up
cleared, rf_mode reset) is performed before kfree(dev->targets).

> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index a92a6566e6a0..950807906645 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;
> +		}
> +	}
> +

[Medium]
The commit message says this teardown mirrors what nfc_deactivate_target()
does, but nfc_deactivate_target() does not touch dev->dep_link_up at all.
Forcing dep_link_up = false here when the active target was set up via
nfc_dep_link_up() introduces a state mutation that is not described in
the commit log.

Can this leave DEP/LLCP state only partially torn down?

Compared to nfc_dep_link_down(), this new branch skips:

	rc = dev->ops->dep_link_down(dev);
	...
	nfc_llcp_mac_is_down(dev);
	nfc_genl_dep_link_down_event(dev);

so the driver is never told to bring the DEP link down, LLCP sockets
bound to the local are not released via nfc_llcp_socket_release(), and
no NFC_EVENT_DEP_LINK_DOWN is sent to userspace.

A subsequent userspace nfc_dep_link_down() will then be rejected here:

net/nfc/core.c:nfc_dep_link_down() {
	...
	if (dev->dep_link_up == false) {
		rc = -EALREADY;
		goto error;
	}
	...
}

so the normal cleanup path that calls nfc_llcp_mac_is_down() is no
longer reachable from userspace until device unregister.

Does this risk leaking the LLCP sockets that were created while
dep_link_up was true?

If the goal is just to clear the dangling active_target pointer, would
it be enough to clear dev->active_target (and stop the presence-check
timer) without also forcing dep_link_up = false and rf_mode = NFC_RF_NONE?
Alternatively, if a full DEP teardown is intended, should this path call
dev->ops->dep_link_down(dev) and nfc_llcp_mac_is_down(dev) like
nfc_dep_link_down() does?