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

[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 3/5] staging: nvec: fix use-after-free in nvec_rx_completed()

[Dan Carpenter]

On Sun, Apr 12, 2026 at 10:51:16PM +0200, Alexandru Hossu wrote:
> 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>

Add a Fixes tag.

> ---
>  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 */

Delete this comment.  (Obvious).

> +		unsigned char msg_type = nvec->rx->data[0];

Move this declaration to the start of the block.

Otherwise, it looks good.

regards,
dan carpenter

> +
>  		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;

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

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