Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Alexandru Hossu]

On Mon, Apr 13, 2026, Dan Carpenter wrote:
> At this point, we're unloading the driver so nvec_power_handle is
> about to be freed. Is there any benefit to setting it to NULL?

nvec_power_off() dereferences nvec_power_handle to send the power-off
command to the EC. If pm_power_off somehow gets reassigned to
nvec_power_off after our driver unloads (e.g. by a re-probe), the stale
nvec_power_handle would point to freed memory.

Setting it to NULL makes the potential failure mode explicit rather than
a silent use-after-free. Since we are already inside the if() guard,
the cost is a single pointer store.

Alexandru

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Dan Carpenter]

On Mon, Apr 13, 2026 at 03:02:50AM -0700, Alexandru Hossu wrote:
> On Mon, Apr 13, 2026, Dan Carpenter wrote:
> > At this point, we're unloading the driver so nvec_power_handle is
> > about to be freed. Is there any benefit to setting it to NULL?
> 
> nvec_power_off() dereferences nvec_power_handle to send the power-off
> command to the EC. If pm_power_off somehow gets reassigned to
> nvec_power_off after our driver unloads (e.g. by a re-probe), the stale
> nvec_power_handle would point to freed memory.

I like to believe it's impossible to reprobe a driver before the
rmmod has completed.  I'm not going to check on this, I'm just going
to take it on faith.  :P

> 
> Setting it to NULL makes the potential failure mode explicit rather than
> a silent use-after-free. Since we are already inside the if() guard,
> the cost is a single pointer store.

So the bug here is that we're racing an rmmod against a poweroff and we
trigger a bug.  And the fix is to change the use after free bug into a
NULL dereference.  Both of rmmod and poweroff are privileged operations
so you kind of get what you deserve if you do that.

I understand that it costs nothing to do the nvec_power_handle = NULL;
and if this were a new driver, I wouldn't comment on it.  (Although I
know other people who would).  But for a new patch, I'm just not sold
on this.  It makes the patch more confusing for no benefit.

regards,
dan carpenter

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Dan Carpenter]

I haven't looked at if you can actually power off a system while an rmmod
is in progress...  I feel like a real old grandpa when I say things like,
"Back in my day you weren't supposed to call rmmod on a production
system, the rmmod feature was only for debugging."

But the bigger picture is that linux power off is a mess.  There are a
dozen different ways to power off a system.  Here we have a driver
which just takes over the power off process by assigning a function
pointer.  Shouldn't we instead have a list of ways to shut down the
system and have a standard way to pick which is the correct one?

Instead of worrying too much about this one driver it would be better
focus on the larger picture.

regards,
dan carpenter

Re: [PATCH 4/5] staging: nvec: fix unconditional pm_power_off teardown

[Alexandru Hossu]

On Mon, Apr 13, 2026, Dan Carpenter wrote:
> Can you resend as a new thread?  Mixing nvec and ipu7 in one series is
> a bit confusing.
>
> Also you can drop the "nvec_power_handle = NULL;" line.  After we set
> pm_power_off = NULL then nvec_power_off() is no longer reachable so
> there is no real bug.

Fair point, thanks. Resent as a clean [PATCH v2 1/2] + [PATCH v2 2/2]
series covering only the nvec fixes. The nvec_power_handle = NULL line
is dropped in v2.

Alexandru

[PATCH 3/5] staging: nvec: fix use-after-free in nvec_rx_completed()

[Alexandru Hossu]

In nvec_rx_completed(), when an incomplete RX transfer is detected,
nvec_msg_free() is called to return the message back to the pool by
clearing its 'used' atomic flag. Immediately after this, the code
accesses nvec->rx->data[0] to check the message type.

Since nvec_msg_free() marks the pool slot as available via atomic_set(),
any concurrent or subsequent call to nvec_msg_alloc() could claim that
same slot and overwrite its data[] array. Reading nvec->rx->data[0] after
freeing the message is therefore a use-after-free.

Fix this by saving the message type byte before calling nvec_msg_free(),
then using the saved value for the battery quirk check.

Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
 drivers/staging/nvec/nvec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 952c5a849a56..75877038847f 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -498,11 +498,14 @@ static void nvec_rx_completed(struct nvec_chip *nvec)
 			(uint)nvec_msg_size(nvec->rx),
 			(uint)nvec->rx->pos);
 
+		/* Save before freeing to avoid use-after-free */
+		unsigned char msg_type = nvec->rx->data[0];
+
 		nvec_msg_free(nvec, nvec->rx);
 		nvec->state = 0;
 
 		/* Battery quirk - Often incomplete, and likes to crash */
-		if (nvec->rx->data[0] == NVEC_BAT)
+		if (msg_type == NVEC_BAT)
 			complete(&nvec->ec_transfer);
 
 		return;
-- 
2.53.0

[PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Alexandru Hossu]

The remove() function unconditionally sets pm_power_off to NULL regardless
of whether this driver instance was the one that set it. There is even a
FIXME comment acknowledging this. Additionally, nvec_power_handle is never
cleared on removal, leaving a dangling pointer to freed device data.

Fix both issues: check that pm_power_off still points to nvec_power_off
before clearing it, and also clear nvec_power_handle at the same time.

Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
 drivers/staging/nvec/nvec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 75877038847f..9fe9b7a3491d 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -907,8 +907,10 @@ static void tegra_nvec_remove(struct platform_device *pdev)
 	nvec_unregister_notifier(nvec, &nvec->nvec_status_notifier);
 	cancel_work_sync(&nvec->rx_work);
 	cancel_work_sync(&nvec->tx_work);
-	/* FIXME: needs check whether nvec is responsible for power off */
-	pm_power_off = NULL;
+	if (pm_power_off == nvec_power_off) {
+		pm_power_off = NULL;
+		nvec_power_handle = NULL;
+	}
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.53.0

Re: [PATCH 4/5] staging: nvec: fix pm_power_off teardown in tegra_nvec_remove()

[Dan Carpenter]

On Sun, Apr 12, 2026 at 10:51:17PM +0200, Alexandru Hossu wrote:
> The remove() function unconditionally sets pm_power_off to NULL regardless
> of whether this driver instance was the one that set it. There is even a
> FIXME comment acknowledging this. Additionally, nvec_power_handle is never
> cleared on removal, leaving a dangling pointer to freed device data.
> 
> Fix both issues: check that pm_power_off still points to nvec_power_off
> before clearing it, and also clear nvec_power_handle at the same time.
> 
> Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
> ---
>  drivers/staging/nvec/nvec.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 75877038847f..9fe9b7a3491d 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -907,8 +907,10 @@ static void tegra_nvec_remove(struct platform_device *pdev)
>  	nvec_unregister_notifier(nvec, &nvec->nvec_status_notifier);
>  	cancel_work_sync(&nvec->rx_work);
>  	cancel_work_sync(&nvec->tx_work);
> -	/* FIXME: needs check whether nvec is responsible for power off */
> -	pm_power_off = NULL;
> +	if (pm_power_off == nvec_power_off) {
> +		pm_power_off = NULL;
> +		nvec_power_handle = NULL;
> +	}

Linux power off handling is a known mess...

I wonder why the original added a comment instead of a test?  To me
checking for if if (pm_power_off == nvec_power_off) makes sense and I
can't see how it would hurt anything.

At this point, we're unloading the driver so nvec_power_handle is
about to be freed.  Is there any benefit to setting it to NULL?

regards,
dan carpenter