linux-kernel - 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: <20250901190140.GO15473@horms.kernel.org>
Date: Mon, 1 Sep 2025 20:01:40 +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 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;
 }
 


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().

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 {

...

-- 
pw-bot: changes-requested