linux-kernel - Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove

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: <496DB504.8090800@s5r6.in-berlin.de>
Date:	Wed, 14 Jan 2009 10:48:52 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	Pekka Enberg <penberg@...helsinki.fi>,
	Manfred Spraul <manfred@...orfullife.com>, krh@...hat.com,
	dcm@....org, Nadia.Derbey@...l.net,
	linux1394-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, paulmck@...ibm.com, stable@...nel.org
Subject: Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all

Andrew Morton wrote:
> On Wed, 14 Jan 2009 09:59:07 +0100 Stefan Richter <stefanr@...6.in-berlin.de> wrote:
> 
>> > --- a/lib/idr.c~lib-idrc-use-kmem_cache_zalloc-for-the-idr_layer-cache
>> > +++ a/lib/idr.c
>> > @@ -121,7 +121,7 @@ int idr_pre_get(struct idr *idp, gfp_t g
>> >  {
>> >  	while (idp->id_free_cnt < IDR_FREE_MAX) {
>> >  		struct idr_layer *new;
>> > -		new = kmem_cache_alloc(idr_layer_cache, gfp_mask);
>> > +		new = kmem_cache_zalloc(idr_layer_cache, gfp_mask);
>> >  		if (new == NULL)
>> >  			return (0);
>> >  		move_to_free_list(idp, new);
>> ...
>> 
>> I wonder if it would be more robust --- or even necessary --- to instead
>> add proper initialization code to get_from_free_list().
>> 
>> As far as David and I tested the new idr using code in firewire, we
>> called idr_remove_all() *and* idr_destroy() before any subsequent
>> idr_get_new().  But in practice, idr_get_new() may of course also happen
>> between idr_remove_all() and idr_destroy().
>> 
>> And then this fix won't be sufficient, would it?
> 
> Maybe I'm having a thick day, but I'm not following you at all here.
> 
> What do you think the remaining problem is?  get_from_free_list()
> starts out with a not-fully-zeroed object?  Something else?

AFAICS:

Before the faulty commit, all code sites which moved something into the
per-idr free list cleared the respective idr_layer.

After the faulty commit, idr_remove_all() forgot to do so.

After your patch, idr_remove_all() still doesn't clear anything.  But
there will typically be idr_destroy() called right after an
idr_remove_all().  idr_destroy() moves all idr_layers out of this idr's
free list back into the kernel-global idr_layer_cache.

Your fix only clears idr_layers which an idr_get_new*() pulls out of the
idr_layer_cache, but not any one which it pulls out of the idr's own
free list.

Hmm, OK.  Seems to be harmless as long as idr users don't start to add
new entries to an idr after they did idr_remove_all().

I presume there is none such user, is there?  Such a usage would be
unusual but not illegal, I suppose.

(Furthermore, I may by thoroughly mistaken about how lib/idr.c works.)
-- 
Stefan Richter
-=====-==--= ---= -===-
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/