Re: [PATCH] hw_random: free rng_buffer at module exit

[Satoru Takeuchi <satoru.takeuchi@gmail.com>]

At Mon, 18 Mar 2013 13:10:29 +1030,
Rusty Russell wrote:
> 
> Ben Hutchings <ben@decadent.org.uk> writes:
> > On Fri, 2013-03-15 at 15:35 +1030, Rusty Russell wrote:
> >> Satoru Takeuchi <satoru.takeuchi@gmail.com> writes:
> >> > At Thu, 14 Mar 2013 17:11:21 +1030,
> >> > Rusty Russell wrote:
> >> >> 
> >> >> Satoru Takeuchi <satoru.takeuchi@gmail.com> writes:
> >> >> > Hi Rusty,
> >> >> >
> >> >> > At Tue, 12 Mar 2013 15:43:33 -0700,
> >> >> > Greg Kroah-Hartman wrote:
> >> >> >> @@ -307,6 +312,14 @@ int hwrng_register(struct hwrng *rng)
> >> >> >>  
> >> >> >>  	mutex_lock(&rng_mutex);
> >> >> >>  
> >> >> >> +	/* kmalloc makes this safe for virt_to_page() in virtio_rng.c */
> >> >> >> +	err = -ENOMEM;
> >> >> >> +	if (!rng_buffer) {
> >> >> >> +		rng_buffer = kmalloc(rng_buffer_size(), GFP_KERNEL);
> >> >> >
> >> >> > rng_buffer is now kmalloc-ed, but not kfree-ed. Shoudn't it be kfree-ed
> >> >> > at hwrng_unregister()? If my suspect is correct, the same problem is
> >> >> > in 3.8.3-rc1 and 3.0.69-rc1. I'm OK to make a patch, but it'll be after
> >> >> > some hours.
> >> >> >
> >> >> > Corecct me if I'm wrong.
> >> >> 
> >> >> Yes, it would be nice to free it, but it really makes sense to free it
> >> >> in module_cleanup() (which would have to be written, as the module
> >> >> currently doesn't have one).
> >> >> 
> >> >> Cheers,
> >> >> Rusty.
> >> >
> >> > From: Satoru Takeuchi <satoru.takeuchi@gmail.com>
> >> >
> >> > rng-core module allocates rng_buffer by kmalloc() since commit
> >> > f7f154f1246ccc5a0a7e9ce50932627d60a0c878. But this buffer won't be
> >> > freed and there is a memory leak possibility at module exit.
> >> >
> >> > Signed-off-by: Satoru Takeuchi <satoru.takeuchi@gmail.com>
> >> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> >> > Cc: Matt Mackall <mpm@selenic.com>
> >> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> >> > Cc: Aurelien Jarno <aurelien@aurel32.net>
> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Cc: stable@vger.kernel.org
> >> 
> >> Cc: stable might be overkill, but I've tested it and put it in my patch
> >> queue, and will push it to Linus once it's survived linux-next.
> >
> > If the module cannot be removed currently, it does not leak.  Making it
> > removable is a feature addition and I think you're right that it's not
> > suitable for stable.
> 
> No, the module has always been removable, but the tiny leak is more a
> theoretical problem I'd say.  AFAICT udev never removes modules, so even
> if you have a randomness device which bounces in and out every second,
> it still won't leak 5MB a day.

I changed my mind. This patch is not suitable for stable because of the
following reasons.

 - Admins (or udev) don't nomally unload hw_random drivers.
 - It's hard for attackers to abuse this bug. Triggering rng-core module
   unload is difficult for non-root users.
 - It leaks few memory (in my system 64byte per load/unload) as Rusty said.

Documentation/stable_kernel_rules.txt
===============================================================================
...
 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).
 - It must fix a problem that causes a build error (but not for things
   marked CONFIG_BROKEN), an oops, a hang, data corruption, a real
   security issue, or some "oh, that's not good" issue.  In short, something
   critical.
...
===============================================================================

It doesn't match the above-mentioned description. It's not critical.

Thanks,
Satoru