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

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

[Xu Wang]

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

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

[Markus Elfring]

> 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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

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

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

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

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 287 bytes --]

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

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]