Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching

[Andreas Gruenbacher <agruen@suse.de>]

On Thursday 12 April 2007 15:46, Andi Kleen wrote:
> > +struct aa_dfa {
> > +	struct table_header *tables[YYTD_ID_NXT];
> > +};
> 
> If that is passed in from user space you would need special compat
> code for 64bit kernels who support 32bit userland.
> Better to avoid pointers.

This struct is not passed between the kernel and user space; no problem here. 
In fact the profiles format used is fully machine independent.

> > +
> > +	/* get optional subprofiles */
> > +	if (aa_is_nameX(e, AA_LIST, "hats")) {
> > +		while (!aa_is_nameX(e, AA_LISTEND, NULL)) {
> > +			struct aa_profile *subprofile;
> > +			subprofile = aa_unpack_profile(e);
> 
> Is there any check that would guard the recursion from stack
> overflow on malicious input?  

It's nice to check for consistency though, so we're adding that. Profile 
loading is a trusted operation, at least so far, and so security wise we 
don't actually have to care --- if loading an invalid profile can bring down 
the system, then that's no worse than an arbitrary module that crashes the 
machine. Not sure if there will ever be user loadable profiles; at least at 
that point we had to care.

> > +	/*
> > +	 * Replacement needs to allocate a new aa_task_context for each
> > +	 * task confined by old_profile.  To do this the profile locks
> > +	 * are only held when the actual switch is done per task.  While
> > +	 * looping to allocate a new aa_task_context the old_task list
> > +	 * may get shorter if tasks exit/change their profile but will
> > +	 * not get longer as new task will not use old_profile detecting
> > +	 * that is stale.
> > +	 */
> > +	do {
> > +		new_cxt = aa_alloc_task_context(GFP_KERNEL | __GFP_NOFAIL);
> 
> NOFAIL is usually a bad sign. It should be only used if there is no
> alternative.

At this point there is no secure alternative to allocating a task context --- 
except killing the task, maybe.

Thanks,
Andreas