linux-kernel - Re: [PATCH] iio: core: fix memleak in iio_device_register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240211193711.518b76d7@jic23-huawei>
Date: Sun, 11 Feb 2024 19:37:11 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: andy.shevchenko@...il.com
Cc: Dinghao Liu <dinghao.liu@....edu.cn>, Lars-Peter Clausen
 <lars@...afoo.de>, Alexandru Ardelean <alexandru.ardelean@...log.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: core: fix memleak in iio_device_register_sysfs

On Sun, 11 Feb 2024 21:16:32 +0200
andy.shevchenko@...il.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@...nel.org> wrote:
> >   
> > > On Fri,  8 Dec 2023 15:31:19 +0800
> > > Dinghao Liu <dinghao.liu@....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@....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