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: <496872E0.9030007@s5r6.in-berlin.de>
Date:	Sat, 10 Jan 2009 11:05:20 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Andrew Morton <akpm@...ux-foundation.org>,
	Kristian Høgsberg 
	<krh@...hat.com>
CC:	dcm@....org, Nadia Derbey <Nadia.Derbey@...l.net>,
	linux1394-devel <linux1394-devel@...ts.sourceforge.net>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ibm.com>,
	Manfred Spraul <manfred@...orfullife.com>
Subject: Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all

Andrew Morton wrote:
> On Sat, 10 Jan 2009 10:03:33 +0100 Stefan Richter <stefanr@...6.in-berlin.de> wrote:
> 
>> David Moore wrote:
>>> From: David Moore <dcm@....org>
>>>
>>> The idr_remove_all() function returns unused slabs to the kmem cache,
>>> but needs to zero them first or else they will be uninitialized upon
>>> next use.  This fixes crashes which have been observed in the firewire
>>> subsystem.
>>>
> 
> hm.
> 
>>> ---
>>>  lib/idr.c |   16 +++++++++++++++-
>>>  1 files changed, 15 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/lib/idr.c b/lib/idr.c
>>> index 1c4f928..69c3455 100644
>>> --- a/lib/idr.c
>>> +++ b/lib/idr.c
>>> @@ -65,6 +65,20 @@ static inline void free_layer(struct idr_layer *p)
>>>  	call_rcu(&p->rcu_head, idr_layer_rcu_free);
>>>  }
>>>  
>>> +static void idr_layer_rcu_free_zero(struct rcu_head *head)
>>> +{
>>> +	struct idr_layer *layer;
>>> +
>>> +	layer = container_of(head, struct idr_layer, rcu_head);
>>> +	memset(layer, 0, sizeof(struct idr_layer));
>>> +	kmem_cache_free(idr_layer_cache, layer);
>>> +}
>>> +
>>> +static inline void free_layer_zero(struct idr_layer *p)
>>> +{
>>> +	call_rcu(&p->rcu_head, idr_layer_rcu_free_zero);
>>> +}
>>> +
>>>  /* only called when idp->lock is held */
>>>  static void __move_to_free_list(struct idr *idp, struct idr_layer *p)
>>>  {
>>> @@ -462,7 +476,7 @@ void idr_remove_all(struct idr *idp)
>>>  		id += 1 << n;
>>>  		while (n < fls(id)) {
>>>  			if (p)
>>> -				free_layer(p);
>>> +				free_layer_zero(p);
>>>  			n += IDR_BITS;
>>>  			p = *--paa;
>>>  		}
>> Nadia,
>>
>> it appears as if post-2.6.26 commit
>> cf481c20c476ad2c0febdace9ce23f5a4db19582 "idr: make idr_remove rcu-safe"
>> was buggy as it removed a memset(...0...) from idr_remove_all() without
>> any obvious replacement.  And this patch fixes it.  Is this correct?
>>
>> This was observed by David in Fedora 2.6.27.* kernels and in 2.6.28, and
>> I have it seen in vanilla 2.6.28 --- but only after I disabled some
>> debug kconfig options.  The trigger for the bug is not the existing
>> usage of idr in drivers/firewire/, but a new usage which is not yet in
>> mainline.  More details:
>> http://marc.info/?l=linux1394-devel&m=123140439522563
>>
>> The symptom is that after a few destructions of idr trees (which involve
>> idr_remove_all() of course), there appear spurious idr entries in
>> subsequently newly created idr trees.  These spurious entries then crash
>> the driver when it iterates over them.

...
> Are we sure that all the other callers of free_layer() are freeing
> zeroed objects?
> 
> It would be cleaner, safer and quite possibly faster to remove the
> constructor altogether and use kmem_cache_zalloc() to allocate new
> objects.

Yes, it sounds at least safer if the allocation path should be fixed up.

The zeroing was done in idr_remove_all() though since Kristian added it
in 2.6.23, until 2.6.26 inclusive.

Kristian, was there a deeper reason to do it at deallocation instead of
allocation, and does the reason still apply today?
-- 
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/