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

Andi Kleen
2007-04-12T12:48:22Z

jjohansen@suse.de writes:

[didn't review code fully, just some stuff I noticed]

> +
> +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.

> +
> + /* 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?

> + /*
> + * 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.

-Andi