linux-kernel - Re: flex_array related problems on selinux policy loading

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]

Date:	Fri, 21 Jan 2011 07:57:35 -0800
From:	Dave Hansen <dave@...ux.vnet.ibm.com>
To:	Steffen Klassert <steffen.klassert@...unet.com>
Cc:	Eric Paris <eparis@...hat.com>, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org
Subject: Re: flex_array related problems on selinux policy loading

On Fri, 2011-01-21 at 08:20 +0100, Steffen Klassert wrote:
> On Thu, Jan 20, 2011 at 07:28:50AM -0800, Dave Hansen wrote:
> > On Thu, 2011-01-20 at 13:26 +0100, Steffen Klassert wrote:
> > ...
> > > @@ -187,6 +195,9 @@ int flex_array_put(struct flex_array *fa, unsigned int element_nr, void *src,
> > >  	struct flex_array_part *part;
> > >  	void *dst;
> > > 
> > > +	if (unlikely(ZERO_OR_NULL_PTR(fa)))
> > > +		return 0;
> > 
> > I think it's OK to add these for the array alloc and free cases.  But,
> > it's really dangerous to do it for put.  It has the potential to
> > silently throw away data and then be really confusing to debug when you
> > can't get it back later.
> 
> If the pointer to struct flex_array is a ZERO_SIZE_PTR we have to exit
> before we try to dereference the first time as we have not allocated
> anything. We can think about returning an error value in flex_array_put
> if the flex_array is a ZERO_SIZE_PTR. The the user would be notified
> that we could not store his data, but that's all we can do here I think.

Yeah, a zero-sized array with a put() done on it should return -ENOSPC,
not 0.  But, an array storing 0-byte objects can and should return 0.

Basically, the patch confuses those two cases.

> > I tend to think about the flex_array itself as being more like a
> > kmem_cache than anything else.  So, all of the operations on the array
> > itself, like shrinking and growing are probably OK.
> 
> Hm, if either element_size or total_nr_elements is zero on allocation time,
> the maximum size the array can ever have is zero. So I don't see how to
> grow (shrink) anything in this case. Do I miss something here?

I mean it shouldn't return an error, nor is it invalid.  You can't _do_
anything, but it's at least valid.

My suggestion would be to simply make sure that the code handles 0-sized
objects and 0-length arrays OK, and do it in two separate patches.  The
ZERO_SIZE_PTR can't be used for both because you need to know which
situation you were in and you need different behavior (like in
flex_array_put()).

Frankly, I like the idea of just allocating a 'struct flex_array' in any
case, and just teaching the code to handle element_size=0 and
nr_elements=0.  That way, if you have bugs in the code that does things
like flex_array_alloc(elem_size=0, len=5, ...) and then
flex_array_get(fa, index=99), you have the potential to detect and
report the bugs.  The only way to do that is to remember what you set
the length as.  

If you're worried about allocating a whole page, you could easily just
kmalloc() a the two integers for the metadata portion of the 'struct
flex_array'.

-- Dave

--
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/