| 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
| ||
|
Message-ID: <4ae3ca7b-dc64-4ab5-b1bf-e357ccc449b4@huawei.com>
Date: Tue, 2 Sep 2025 09:41:33 +0800
From: Wang Liang <wangliang74@...wei.com>
To: Simon Horman <horms@...nel.org>
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
在 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.
>
>
> 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?
------
Best regards
Wang Liang
> If so, I wonder if this can be resolved (in a separate patch to
> the fix for atm_register_sysfs()) like this.
> (Also compile tested only!)
>
> diff --git a/net/atm/resources.c b/net/atm/resources.c
> index b19d851e1f44..3002ff5b60f8 100644
> --- a/net/atm/resources.c
> +++ b/net/atm/resources.c
> @@ -112,13 +110,12 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
>
> if (atm_proc_dev_register(dev) < 0) {
> pr_err("atm_proc_dev_register failed for dev %s\n", type);
> - goto out_fail;
> + goto err_free_dev;
> }
>
> if (atm_register_sysfs(dev, parent) < 0) {
> pr_err("atm_register_sysfs failed for dev %s\n", type);
> - atm_proc_dev_deregister(dev);
> - goto out_fail;
> + goto err_proc_dev_unregister;
> }
>
> list_add_tail(&dev->dev_list, &atm_devs);
> @@ -127,7 +124,9 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
> mutex_unlock(&atm_dev_mutex);
> return dev;
>
> -out_fail:
> +err_proc_dev_unregister:
> + atm_proc_dev_deregister(dev);
> +err_free_dev:
> kfree(dev);
> dev = NULL;
> goto out;
>
> Lastly, while not a bug and not material for net, it would be nice to
> follow-up on the above and consolidate the error handling in
> atm_dev_register().
>
> Something like this (compile tested only!):
>
> diff --git a/net/atm/resources.c b/net/atm/resources.c
> index b19d851e1f44..3002ff5b60f8 100644
> --- a/net/atm/resources.c
> +++ b/net/atm/resources.c
> @@ -89,9 +89,7 @@ struct atm_dev *atm_dev_register(const char *type, struct device *parent,
> inuse = __atm_dev_lookup(number);
> if (inuse) {
> atm_dev_put(inuse);
> - mutex_unlock(&atm_dev_mutex);
> - kfree(dev);
> - return NULL;
> + goto err_free_dev;
> }
> dev->number = number;
> } else {
>
> ...
>
Powered by blists - more mailing lists