On Mon, Jan 31, 2011 at 08:31:37AM -0800, Dave Hansen wrote:
>
> I think this still has some of the issues of the earlier patch. The
> zero-size check needs to be moved after the ->total_nr_elements check.
> Otherwise, this won't produce any errors:
>
> fa = flex_array_alloc(0, 100, GFP_KERNEL);
> flex_array_put(fa, 1001, ptr, GFP_KERNEL);
>
> > @@ -284,6 +297,8 @@ void *flex_array_get(struct flex_array *fa, unsigned int element_nr)
> > int part_nr = fa_element_to_part_nr(fa, element_nr);
> > struct flex_array_part *part;
> >
> > + if (!fa->total_nr_elements || !fa->element_size)
> > + return NULL;
> > if (element_nr >= fa->total_nr_elements)
> > return NULL;
> > if (elements_fit_in_base(fa))
>
> Do you really need to check fa->total_nr_elements both for zero and
> against element_nr? Seems a but superfluous to me.
>
Both objections are correct, I'll send an updated patch.
Steffen
On Tue, Feb 01, 2011 at 11:24:18AM +0100, Steffen Klassert wrote:
>
> Both objections are correct, I'll send an updated patch.
>
I think we need to fix selinux too to get rid of the policy loading
problem. In security/selinux/ss/policydb.c are several pieces of code
like this one:
p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
p->p_types.nprim,
GFP_KERNEL | __GFP_ZERO);
if (!p->type_val_to_struct_array)
goto out;
rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
p->p_types.nprim - 1, GFP_KERNEL | __GFP_ZERO);
if (rc)
goto out;
If p->p_types.nprim is zero, we allocare with total_nr_elements equal
to zerro and then we try to prealloc with p->p_types.nprim - 1.
flex_array_prealloc interprets this as an unsigned int and fails,
because this is bigger than total_nr_elements, which is correct I think.
Thoughts?
Steffen
On Tue, 2011-02-01 at 12:03 +0100, Steffen Klassert wrote:
> rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
> p->p_types.nprim - 1, GFP_KERNEL | __GFP_ZERO);
> if (rc)
> goto out;
>
> If p->p_types.nprim is zero, we allocare with total_nr_elements equal
> to zerro and then we try to prealloc with p->p_types.nprim - 1.
> flex_array_prealloc interprets this as an unsigned int and fails,
> because this is bigger than total_nr_elements, which is correct I
> think.
>
> Thoughts?
The most we ever hold in a flex_array is ~2 million entries. So we have
plenty of room to use a normal int if you want.
On the other hand, there's only one user of flex_array_prealloc(), and
making the "end" argument inclusive doesn't seem to be what that user
wants. We might want to either make flex_array_prealloc() take start
and length, or instead make "end" be exclusive of the "end" index.
I thought that flex_array_prealloc would say, effectively: "all put()'s
would work up until 'end'". But, looking at it now, that's probably not
how people will use it.
-- Dave
On Tue, 2011-02-01 at 06:55 -0800, Dave Hansen wrote:
> On Tue, 2011-02-01 at 12:03 +0100, Steffen Klassert wrote:
> > rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
> > p->p_types.nprim - 1, GFP_KERNEL | __GFP_ZERO);
> > if (rc)
> > goto out;
> >
> > If p->p_types.nprim is zero, we allocare with total_nr_elements equal
> > to zerro and then we try to prealloc with p->p_types.nprim - 1.
> > flex_array_prealloc interprets this as an unsigned int and fails,
> > because this is bigger than total_nr_elements, which is correct I
> > think.
> >
> > Thoughts?
>
> The most we ever hold in a flex_array is ~2 million entries. So we have
> plenty of room to use a normal int if you want.
>
> On the other hand, there's only one user of flex_array_prealloc(), and
> making the "end" argument inclusive doesn't seem to be what that user
> wants. We might want to either make flex_array_prealloc() take start
> and length, or instead make "end" be exclusive of the "end" index.
>
> I thought that flex_array_prealloc would say, effectively: "all put()'s
> would work up until 'end'". But, looking at it now, that's probably not
> how people will use it.
I'm fine with any solution. It's obviously broken for SELinux to be
passing -1 even if the library supported it. I guess I don't really
have strong feelings on how to fix it.
1) make end exclusive
2) change 'end' to 'len'
3) just make selinux not prealloc() when the #elements == 0
All seem perfectly reasonable to me, but I'd probably do them in that
order.
-Eric
On Tue, Feb 01, 2011 at 10:20:22AM -0500, Eric Paris wrote:
>
> I'm fine with any solution. It's obviously broken for SELinux to be
> passing -1 even if the library supported it. I guess I don't really
> have strong feelings on how to fix it.
>
> 1) make end exclusive
> 2) change 'end' to 'len'
> 3) just make selinux not prealloc() when the #elements == 0
>
> All seem perfectly reasonable to me, but I'd probably do them in that
> order.
>
I think I would prefer option 2. Passing a size or a length to
a memory allocator is quite common and we would not need to care
for bugs where 'end' is smaller than 'start'.
So I'd do it like that, if noone has strong feelings for another option.
Steffen