[PATCH 2/2] AppArmor: Allow dfa backward compatibility with broken userspace

Tim Gardner tim.gardner at canonical.com
Mon Dec 6 20:16:16 UTC 2010


On 12/03/2010 03:14 PM, John Johansen wrote:
> BugLink: http://bugs.launchpad.net/bugs/581525
>
> SRU Justification:
>     See bug link for full justification
>
> NOTE:
>     This patch is OPTIONAL, and is only needed if the user space tools have
>     not been updated.  The userspace tools update has been pulled in, so
>     unless there is the need to support users updating the kernel without
>     updating the userspace tools, I am NOT recommending this patch for
>     SRU, but including it for completeness.
>
>     If the the patch to fix the bounds check on the next/check table has
>     been applied, and the userspace has not been updated.  The it is likely
>     that part or all of policy will fail to load due to the bounds check
>     rejecting policy.
>
>     The Justification for this patch is that it supports using a non-updated
>     (broken) userspace with a kernel that has the kernel fix for bug#581525
>     applied.
>
> The apparmor_parser when compiling policy could generate invalid dfas
> that did not have sufficient padding to avoid invalid references, when
> used by the kernel.  The kernels check to verify the next/check table
> size was broken meaning invalid dfas were being created by userspace
> and not caught.
>
> To remain compatible with old tools that are not fixed, pad the loaded
> dfas next/check table.  The dfa's themselves are valid except for the
> high padding for potentially invalid transitions (high bounds error),
> which have a maximimum is 256 entries.  So just allocate an extra null filled
> 256 entries for the next/check tables.  This will guarentee all bounds
> are good and invalid transitions go to the null (0) state.
>
> Signed-off-by: John Johansen<john.johansen at canonical.com>
> ---
>   security/apparmor/include/apparmor.h |    6 ++++++
>   security/apparmor/match.c            |    9 ++++++++-
>   2 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h
> index 3f8d866..b4ddc95 100644
> --- a/security/apparmor/include/apparmor.h
> +++ b/security/apparmor/include/apparmor.h
> @@ -46,6 +46,12 @@ extern unsigned int aa_g_path_max;
>   			printk(KERN_ERR "AppArmor: " fmt, ##args);	\
>   	} while (0)
>
> +#define AA_WARN(fmt, args...)						\
> +	do {								\
> +		if (printk_ratelimit())					\
> +			printk(KERN_WARN "AppArmor: " fmt, ##args);	\
> +	} while (0)
> +
>   /* Flag indicating whether initialization completed */
>   extern int apparmor_initialized;
>   void apparmor_disable(void);
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index b456495..f4486c1 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -91,6 +91,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>   	if (bsize<  tsize)
>   		goto out;
>
> +	/* Pad table allocation for next/check by 256 entries to remain
> +	 * backwards compatible with old (buggy) tools and remain safe without
> +	 * run time checks
> +	 */
> +	if (th.td_id == YYTD_ID_NXT || th.td_id == YYTD_ID_CHK)
> +		tsize += 256 * th.td_flags;
> +
>   	/* freed by free_table */
>   	table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
>   	if (!table) {
> @@ -178,7 +185,7 @@ static int verify_dfa(struct aa_dfa *dfa, int flags)
>   				goto out;
>   			/* TODO: do check that DEF state recursion terminates */
>   			if (BASE_TABLE(dfa)[i] + 255>= trans_count) {
> -				printk("AppArmor DFA next/check upper bounds error\n");
> +				AA_WARN("DFA next/check upper bounds fixed, upgrade user space tools\n");
>   				goto out;
>   			}
>   		}

I'm having a hard time parsing the commit log message. Are you trying to 
say that the Lucid kernel won't work if the current Maverick apparmor 
user space is backported to Lucid? Confused...

rtg
-- 
Tim Gardner tim.gardner at canonical.com




More information about the kernel-team mailing list