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] [day] [month] [year] [list]

Message-ID: <CALGdzurn0q49m10=K8vPDnvWyw9UyuaPVbYDnuE71oFp0TAAeQ@mail.gmail.com>
Date: Sat, 16 Mar 2024 10:11:23 -0500
From: Chenyuan Yang <chenyuan0y@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, "fw@...len.de" <fw@...len.de>, kuniyu@...zon.com
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, horms@...nel.org, anjali.k.kulkarni@...cle.com, 
	pctammela@...atatu.com, dhowells@...hat.com, netdev@...r.kernel.org, 
	zzjas98@...il.com
Subject: Re: [net/netlink] Question about potential memleak in netlink_proto_init()

Thank you all for sharing your insights on this issue.

I'm pondering over the best solution: should we follow Kuniyuki's
suggestion to "eliminate the cleanup code," or would it be better to
adopt Florian's approach of "eschewing error handling in favor of
immediate panic"?

Best,
Chenyuan

On Fri, Mar 15, 2024 at 9:36 AM Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
>
> On Thu, Mar 14, 2024 at 07:47:18PM -0500, Chenyuan Yang wrote:
> > 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;
> >               }
> >       }
> >   ...
> > }
> > ```
> >
> > 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)
> > ```
>
> The better pattern (and widely used in kernel) is
>
>         while (i--)
>
> > Please kindly correct us if we missed any key information. Looking forward to
> > your response!
>
> --
> With Best Regards,
> Andy Shevchenko
>
>