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

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

[Xu Wang]

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 <vulab@iscas.ac.cn>
---
 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

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

[Markus Elfring]

> 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

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

[Markus Elfring]

> 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

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

[Markus Elfring]

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

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

[Markus Elfring]

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

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

[Srinivas Kandagatla]

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 <vulab@iscas.ac.cn>
> ---
>   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);
>   }
>   
>