netdev - Re: [net/netlink] Question about potential memleak in netlink_proto

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]

Date: Thu, 14 Mar 2024 17:54:58 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <chenyuan0y@...il.com>
CC: <andriy.shevchenko@...ux.intel.com>, <anjali.k.kulkarni@...cle.com>,
	<davem@...emloft.net>, <dhowells@...hat.com>, <edumazet@...gle.com>,
	<horms@...nel.org>, <kuba@...nel.org>, <kuniyu@...zon.com>,
	<netdev@...r.kernel.org>, <pabeni@...hat.com>, <pctammela@...atatu.com>,
	<zzjas98@...il.com>
Subject: Re: [net/netlink] Question about potential memleak in netlink_proto_init()

From: Chenyuan Yang <chenyuan0y@...il.com>
Date: Thu, 14 Mar 2024 19:47:18 -0500
> Dear Netlink Developers,
> 
> We are curious whether the function `netlink_proto_init()` might have a memory leak.
> 
> The function is https://elixir.bootlin.com/linux/v6.8/source/net/netlink/af_netlink.c#L2908
> and the relevant code is
> ```
> static int __init netlink_proto_init(void)
> {
> 	int i;
>   ...
> 
> 	for (i = 0; i < MAX_LINKS; i++) {
> 		if (rhashtable_init(&nl_table[i].hash,
> 				    &netlink_rhashtable_params) < 0) {
> 			while (--i > 0)
> 				rhashtable_destroy(&nl_table[i].hash);
> 			kfree(nl_table);
> 			goto panic;

If rhashtable_init() fails, the kernel panic occurs, so there's
no real memleak issue.


> 		}
> 	}
>   ...
> }
> ```
> 
> In the for loop, when `rhashtable_init()` fails, the function will free
> the allocated memory for `nl_table[i].hash` by checking `while (--i > 0)`.
> However, the first element (`i=1`) of `nl_table` is not freed since `i` is
> decremented before the check.
> 
> Based on our understanding, a possible fix would be
> ```
> -      while (--i > 0)
> +      while (--i >= 0)
> ```

Change itself looks good, no need for cleanup in the first place though.