2019-12-27 09:38:33

by Xu Wang

[permalink] [raw]
Subject: [PATCH 2/2] i2c: Fix a potential use after free

Free the adap 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/i2c/i2c-core-base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9f8dcd3..160d43e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -2301,8 +2301,8 @@ void i2c_put_adapter(struct i2c_adapter *adap)
if (!adap)
return;

- put_device(&adap->dev);
module_put(adap->owner);
+ put_device(&adap->dev);
}
EXPORT_SYMBOL(i2c_put_adapter);

--
2.7.4


2019-12-28 12:53:17

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH 2/2] i2c: Fix a potential use after free

> Free the adap 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.
Would you like to clarify the dependencies for such an use case any more?

Regards,
Markus

2020-03-22 15:58:24

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH 2/2] i2c: Fix a potential use after free

On Fri, Dec 27, 2019 at 09:34:32AM +0000, Xu Wang wrote:
> Free the adap 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]>

Do you have a testcase to reproduce it?

I wonder because we are freeing the device structure which is embedded
inside the adap structure, not the adap structure itself. Or?

> ---
> drivers/i2c/i2c-core-base.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9f8dcd3..160d43e 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2301,8 +2301,8 @@ void i2c_put_adapter(struct i2c_adapter *adap)
> if (!adap)
> return;
>
> - put_device(&adap->dev);
> module_put(adap->owner);
> + put_device(&adap->dev);
> }
> EXPORT_SYMBOL(i2c_put_adapter);
>
> --
> 2.7.4
>


Attachments:
(No filename) (978.00 B)
signature.asc (849.00 B)
Download all attachments

2022-06-16 21:03:54

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH 2/2] i2c: Fix a potential use after free

On Fri, Dec 27, 2019 at 09:34:32AM +0000, Xu Wang wrote:
> Free the adap 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]>

Added a comment why we reverse the order for putting our stuff and
applied to for-next, thanks! This way, we get more testing until it hits
upstream. Still, stable tag added because we want it to be backported if
all is well.


Attachments:
(No filename) (487.00 B)
signature.asc (849.00 B)
Download all attachments

2022-07-26 21:48:33

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH 2/2] i2c: Fix a potential use after free

On Fri, Dec 27, 2019 at 09:34:32AM +0000, Xu Wang wrote:
> Free the adap 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]>

Applied to for-next, thanks!


Attachments:
(No filename) (287.00 B)
signature.asc (849.00 B)
Download all attachments