[PATCH] iio: core: fix memleak in iio_device_register_sysfs

[PATCH] iio: core: fix memleak in iio_device_register_sysfs

[Dinghao Liu]

When iio_device_register_sysfs_group() fails, we should
free iio_dev_opaque->chan_attr_group.attrs to prevent
potential memleak.

Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---
 drivers/iio/industrialio-core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index c77745b594bd..e6d3d07a4c83 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1581,10 +1581,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 	ret = iio_device_register_sysfs_group(indio_dev,
 					      &iio_dev_opaque->chan_attr_group);
 	if (ret)
-		goto error_clear_attrs;
+		goto error_free_chan_attrs;
 
 	return 0;
 
+error_free_chan_attrs:
+	kfree(iio_dev_opaque->chan_attr_group.attrs);
+	iio_dev_opaque->chan_attr_group.attrs = NULL;
 error_clear_attrs:
 	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
 
-- 
2.17.1

Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs

[Jonathan Cameron]

On Fri,  8 Dec 2023 15:31:19 +0800
Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:

> When iio_device_register_sysfs_group() fails, we should
> free iio_dev_opaque->chan_attr_group.attrs to prevent
> potential memleak.
> 
> Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
Hi.

Looks good to me, but I'd like to leave this one on the list a little
longer to see if anyone else has comments.

Jonathan

> ---
>  drivers/iio/industrialio-core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index c77745b594bd..e6d3d07a4c83 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -1581,10 +1581,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
>  	ret = iio_device_register_sysfs_group(indio_dev,
>  					      &iio_dev_opaque->chan_attr_group);
>  	if (ret)
> -		goto error_clear_attrs;
> +		goto error_free_chan_attrs;
>  
>  	return 0;
>  
> +error_free_chan_attrs:
> +	kfree(iio_dev_opaque->chan_attr_group.attrs);
> +	iio_dev_opaque->chan_attr_group.attrs = NULL;
>  error_clear_attrs:
>  	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
>  

Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs

[Jonathan Cameron]

On Sun, 10 Dec 2023 13:32:28 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri,  8 Dec 2023 15:31:19 +0800
> Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> 
> > When iio_device_register_sysfs_group() fails, we should
> > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > potential memleak.
> > 
> > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>  
> Hi.
> 
> Looks good to me, but I'd like to leave this one on the list a little
> longer to see if anyone else has comments.
> 
Guess no comments!

Applied to the fixes-togreg branch of iio.git.  I might not get another
fixes request out far enough in advance of the merge window in which case
this will go in around then instead.

Also marked for stable

Jonathan

> Jonathan
> 
> > ---
> >  drivers/iio/industrialio-core.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index c77745b594bd..e6d3d07a4c83 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1581,10 +1581,13 @@ static int iio_device_register_sysfs(struct iio_dev *indio_dev)
> >  	ret = iio_device_register_sysfs_group(indio_dev,
> >  					      &iio_dev_opaque->chan_attr_group);
> >  	if (ret)
> > -		goto error_clear_attrs;
> > +		goto error_free_chan_attrs;
> >  
> >  	return 0;
> >  
> > +error_free_chan_attrs:
> > +	kfree(iio_dev_opaque->chan_attr_group.attrs);
> > +	iio_dev_opaque->chan_attr_group.attrs = NULL;
> >  error_clear_attrs:
> >  	iio_free_chan_devattr_list(&iio_dev_opaque->channel_attr_list);
> >    
> 
> 

Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs

[andy.shevchenko]

Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> On Sun, 10 Dec 2023 13:32:28 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri,  8 Dec 2023 15:31:19 +0800
> > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > 
> > > When iio_device_register_sysfs_group() fails, we should
> > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > potential memleak.
> > > 
> > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>  
> > Hi.
> > 
> > Looks good to me, but I'd like to leave this one on the list a little
> > longer to see if anyone else has comments.
> > 
> Guess no comments!

This patch does not fix anything.

Yet, it might be considered as one that increases robustness, but with this applied the 
goto
https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
can be amended, right?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs

[Jonathan Cameron]

On Sun, 11 Feb 2024 21:16:32 +0200
andy.shevchenko@gmail.com wrote:

> Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > On Sun, 10 Dec 2023 13:32:28 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Fri,  8 Dec 2023 15:31:19 +0800
> > > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > >   
> > > > When iio_device_register_sysfs_group() fails, we should
> > > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > > potential memleak.
> > > > 
> > > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>    
> > > Hi.
> > > 
> > > Looks good to me, but I'd like to leave this one on the list a little
> > > longer to see if anyone else has comments.
> > >   
> > Guess no comments!  
> 
> This patch does not fix anything.
> 
> Yet, it might be considered as one that increases robustness, but with this applied the 
> goto
> https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
> can be amended, right?
> 

I'm lost.  That goto results in a call to 
iio_buffers_free_sysfs_and_mask(indio_dev);
which continues to undo stuff from before that call.
Now if it did
iio_device_unregister_sysfs(indio_dev);
(as per the label above it in the cleanup) then I'd agree.

Perhaps I'm misreading the code flow here?

All this code is supposed to be side effect free on error so I'm
keen on the change even if there is another path where it gets cleaned
up that I'm missing.

Jonathan

Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs

[Andy Shevchenko]

On Sun, Feb 11, 2024 at 9:37 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 11 Feb 2024 21:16:32 +0200
> andy.shevchenko@gmail.com wrote:
>
> > Sun, Dec 17, 2023 at 01:28:00PM +0000, Jonathan Cameron kirjoitti:
> > > On Sun, 10 Dec 2023 13:32:28 +0000
> > > Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > > On Fri,  8 Dec 2023 15:31:19 +0800
> > > > Dinghao Liu <dinghao.liu@zju.edu.cn> wrote:
> > > >
> > > > > When iio_device_register_sysfs_group() fails, we should
> > > > > free iio_dev_opaque->chan_attr_group.attrs to prevent
> > > > > potential memleak.
> > > > >
> > > > > Fixes: 32f171724e5c ("iio: core: rework iio device group creation")
> > > > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> > > > Hi.
> > > >
> > > > Looks good to me, but I'd like to leave this one on the list a little
> > > > longer to see if anyone else has comments.
> > > >
> > > Guess no comments!
> >
> > This patch does not fix anything.
> >
> > Yet, it might be considered as one that increases robustness, but with this applied the
> > goto
> > https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/iio/industrialio-core.c#L2007
> > can be amended, right?
> >
>
> I'm lost.  That goto results in a call to
> iio_buffers_free_sysfs_and_mask(indio_dev);
> which continues to undo stuff from before that call.
> Now if it did
> iio_device_unregister_sysfs(indio_dev);
> (as per the label above it in the cleanup) then I'd agree.

Ah, it's me who hasn't noticed the word "buffer" in the goto label.
Yes, you are right!

> Perhaps I'm misreading the code flow here?
>
> All this code is supposed to be side effect free on error so I'm
> keen on the change even if there is another path where it gets cleaned
> up that I'm missing.

Everything is fine, sorry for the noise.

-- 
With Best Regards,
Andy Shevchenko