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

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: <p733b35903l.fsf@bingen.suse.de>
Date:	12 Apr 2007 15:46:38 +0200
From:	Andi Kleen <andi@...stfloor.org>
To:	jjohansen@...e.de
Cc:	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, chrisw@...s-sol.org,
	Andreas Gruenbacher <agruen@...e.de>
Subject: Re: [AppArmor 39/41] AppArmor: Profile loading and manipulation, pathname matching

jjohansen@...e.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

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