Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934372Ab3CTAco (ORCPT ); Tue, 19 Mar 2013 20:32:44 -0400 Received: from mail-da0-f43.google.com ([209.85.210.43]:44871 "EHLO mail-da0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932902Ab3CTAcl (ORCPT ); Tue, 19 Mar 2013 20:32:41 -0400 Date: Wed, 20 Mar 2013 09:29:57 +0900 Message-ID: <87li9i9am2.wl%satoru.takeuchi@gmail.com> From: Satoru Takeuchi To: Rusty Russell Cc: Ben Hutchings , Satoru Takeuchi , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, stable@vger.kernel.org, Aurelien Jarno , Matt Mackall , Herbert Xu Subject: Re: [PATCH] hw_random: free rng_buffer at module exit In-Reply-To: <87ppyxsa5e.fsf@rustcorp.com.au> References: <20130312223211.492954675@linuxfoundation.org> <20130312223212.790278686@linuxfoundation.org> <874ngeykp6.wl%satoru.takeuchi@gmail.com> <87txoetre6.fsf@rustcorp.com.au> <87k3pagllu.wl%satoru.takeuchi@gmail.com> <87d2v1tfr0.fsf@rustcorp.com.au> <1363486492.3937.238.camel@deadeye.wl.decadent.org.uk> <87ppyxsa5e.fsf@rustcorp.com.au> User-Agent: Wanderlust/2.14.0 (Africa) Emacs/23.4 Mule/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3812 Lines: 92 At Mon, 18 Mar 2013 13:10:29 +1030, Rusty Russell wrote: > > Ben Hutchings writes: > > On Fri, 2013-03-15 at 15:35 +1030, Rusty Russell wrote: > >> Satoru Takeuchi writes: > >> > At Thu, 14 Mar 2013 17:11:21 +1030, > >> > Rusty Russell wrote: > >> >> > >> >> Satoru Takeuchi 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 > >> > > >> > 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 > >> > Cc: Rusty Russell > >> > Cc: Matt Mackall > >> > Cc: Herbert Xu > >> > Cc: Aurelien Jarno > >> > Cc: Greg Kroah-Hartman > >> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/