2019-12-27 09:22:01

by Xu Wang

Subject: [PATCH] nvmem: core: Fix a potential use after free

Free the nvmem structure only after we are done using it.
This patch just moves the put_device() down a bit to avoid the
use after free.

Signed-off-by: Xu Wang <[email protected]>
---
drivers/nvmem/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9f1ee9c..7051d34 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -535,8 +535,8 @@ static struct nvmem_device *__nvmem_device_get(void *data,

static void __nvmem_device_put(struct nvmem_device *nvmem)
{
- put_device(&nvmem->dev);
module_put(nvmem->owner);
+ put_device(&nvmem->dev);
kref_put(&nvmem->refcnt, nvmem_device_release);
}

--
2.7.4

2019-12-28 11:32:22

by Markus Elfring

Subject: Re: [PATCH] nvmem: core: Fix a potential use after free

> Free the nvmem structure only after we are done using it.

This information can be reasonable.


> This patch just moves the put_device() down a bit to avoid the
> use after free.

I suggest to reconsider such a change because a device reference count
should eventually be decremented before decrementing the reference count
for the module which is managed by this programming interface.
Are you also looking for better software documentation for such an use case?

Regards,
Markus

2019-12-28 12:30:54

by Markus Elfring

Subject: Re: nvmem: core: Checking the decrementing of reference counters

I have taken another look at the implementation of the function “nvmem_device_release”.
https://elixir.bootlin.com/linux/v5.5-rc3/source/drivers/nvmem/core.c#L421
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvmem/core.c?id=bf8d1cd4386535004c4afe7f03d37f9864c9940e#n421

Now I wonder why the statement “put_device(&nvmem->dev)” is performed here
after it was also executed by the function “__nvmem_device_put” before.
How often should the device reference count be decremented (at the end)?

Regards,
Markus

2020-01-06 12:36:31

by Srinivas Kandagatla

Subject: Re: [PATCH] nvmem: core: Fix a potential use after free

Thanks for the patch.

On 27/12/2019 09:20, Xu Wang wrote:
> Free the nvmem structure only after we are done using it.
> This patch just moves the put_device() down a bit to avoid the
> use after free.

Could you explain the issue bit more here on what exactly could go wrong
with the exiting order?
may be the stack trace of the use-after-free case? Or steps to reproduce
the issue?

nvmem device is protected with kref.

--srini

>
> Signed-off-by: Xu Wang <[email protected]>
> ---
> drivers/nvmem/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 9f1ee9c..7051d34 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -535,8 +535,8 @@ static struct nvmem_device *__nvmem_device_get(void *data,
>
> static void __nvmem_device_put(struct nvmem_device *nvmem)
> {
> - put_device(&nvmem->dev);
> module_put(nvmem->owner);
> + put_device(&nvmem->dev);
> kref_put(&nvmem->refcnt, nvmem_device_release);
> }
>
>