netdev - Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when device

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: <20250902100716.GB15473@horms.kernel.org>
Date: Tue, 2 Sep 2025 11:07:16 +0100
From: Simon Horman <horms@...nel.org>
To: Wang Liang <wangliang74@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, kuniyu@...gle.com, kay.sievers@...y.org,
	gregkh@...e.de, yuehaibing@...wei.com, zhangchangzhong@...wei.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: atm: fix memory leak in atm_register_sysfs when
 device_register fail

On Tue, Sep 02, 2025 at 09:41:33AM +0800, Wang Liang wrote:
> 
> 在 2025/9/2 3:01, Simon Horman 写道:
> > On Mon, Sep 01, 2025 at 02:35:37PM +0800, Wang Liang wrote:
> > > When device_register() return error in atm_register_sysfs(), which can be
> > > triggered by kzalloc fail in device_private_init() or other reasons,
> > > kmemleak reports the following memory leaks:
> > > 
> > > unreferenced object 0xffff88810182fb80 (size 8):
> > >    comm "insmod", pid 504, jiffies 4294852464
> > >    hex dump (first 8 bytes):
> > >      61 64 75 6d 6d 79 30 00                          adummy0.
> > >    backtrace (crc 14dfadaf):
> > >      __kmalloc_node_track_caller_noprof+0x335/0x450
> > >      kvasprintf+0xb3/0x130
> > >      kobject_set_name_vargs+0x45/0x120
> > >      dev_set_name+0xa9/0xe0
> > >      atm_register_sysfs+0xf3/0x220
> > >      atm_dev_register+0x40b/0x780
> > >      0xffffffffa000b089
> > >      do_one_initcall+0x89/0x300
> > >      do_init_module+0x27b/0x7d0
> > >      load_module+0x54cd/0x5ff0
> > >      init_module_from_file+0xe4/0x150
> > >      idempotent_init_module+0x32c/0x610
> > >      __x64_sys_finit_module+0xbd/0x120
> > >      do_syscall_64+0xa8/0x270
> > >      entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > 
> > > When device_create_file() return error in atm_register_sysfs(), the same
> > > issue also can be triggered.
> > > 
> > > Function put_device() should be called to release kobj->name memory and
> > > other device resource, instead of kfree().
> > > 
> > > Fixes: 1fa5ae857bb1 ("driver core: get rid of struct device's bus_id string array")
> > > Signed-off-by: Wang Liang <wangliang74@...wei.com>
> > Thanks Wang Liang,
> > 
> > I agree this is a bug.
> > 
> > I think that the guiding principle should be that on error functions
> > unwind any resource allocations they have made, rather than leaving
> > it up to callers to clean things up.
> > 
> > So, as the problem you describe seems to be due to atm_register_sysfs()
> > leaking resources if it encounters an error, I think the problem would
> > best be resolved there.
> > 
> > Perhaps something like this.
> > (Compile tested only!)
> > 
> > diff --git a/net/atm/atm_sysfs.c b/net/atm/atm_sysfs.c
> > index 54e7fb1a4ee5..62f3d520a80a 100644
> > --- a/net/atm/atm_sysfs.c
> > +++ b/net/atm/atm_sysfs.c
> > @@ -148,20 +148,23 @@ int atm_register_sysfs(struct atm_dev *adev, struct device *parent)
> >   	dev_set_name(cdev, "%s%d", adev->type, adev->number);
> >   	err = device_register(cdev);
> >   	if (err < 0)
> > -		return err;
> > +		goto err_put_dev;
> >   	for (i = 0; atm_attrs[i]; i++) {
> >   		err = device_create_file(cdev, atm_attrs[i]);
> >   		if (err)
> > -			goto err_out;
> > +			goto err_remove_file;
> >   	}
> >   	return 0;
> > -err_out:
> > +err_remove_file:
> >   	for (j = 0; j < i; j++)
> >   		device_remove_file(cdev, atm_attrs[j]);
> >   	device_del(cdev);
> > +err_put_dev:
> > +	put_device(cdev);
> > +
> >   	return err;
> >   }
> 
> 
> Thanks for your replies, it is very clear!
> 
> But the above code may introduce a use-after-free issue. If
> device_register()
> fails, put_device() call atm_release() to free atm_dev, and
> atm_proc_dev_deregister() will visit it.
> 
> And kfree() should be removed in atm_dev_register() to avoid double-free.

Thanks, I see that now.

I do think that it would be nice to untangle the error handling here.
But, as a but fix I now think your original approach is good.
Because it addresses the issue in a minimal way.

Reviewed-by: Simon Horman <horms@...nel.org>

> > Looking over atm_dev_register, it seems to me that it will deadlock
> > if it calls atm_proc_dev_deregister() if atm_register_sysfs() fails.
> > This is because atm_dev_register() is holding atm_dev_mutex,
> > and atm_proc_dev_deregister() tries to take atm_dev_mutex().
> 
> 
> I cannot find somewhere tries to take atm_dev_mutex(), can you give some
> hints?

Sorry, my mistake. I was looking at atm_dev_deregister()
rather than atm_proc_dev_deregister().

...

-- 
pw-bot: under-review