[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 <ddd...@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

[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?

[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?