Patchwork [08/11] AppArmor: Make sure to unmap aliases for vmalloced dfas before they are live

login
register
mail settings
Submitter John Johansen
Date April 13, 2010, 7:09 a.m.
Message ID <1271142580-26555-9-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/50042/
State Accepted
Delegated to: Andy Whitcroft
Headers show

Comments

John Johansen - April 13, 2010, 7:09 a.m.
From: John Johansen <john.johansen@canonical.com>

OriginalAuthor: John Johansen <john.johansen@canonical.com>
OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
commit: 6857acf643ba19eddfc29125fc011a3ce48fe87b
BugLink: http://bugs.launchpad.net/bugs/529288

vmalloc doesn't guarentee that the tlbs of all cpus will be flushed
when it completes.  Instead the tlbs gets flushed lazily, however for
AppArmor this is a problem as the dfa becomes live to all cpus as
soon as the profile replacedby value is written (this is even before
locking of the lists are removed).

It is possible for another cpu to be in a state where it has an old
tlb mapping for the vmalloc address (this will be caused by putting
a reference on an old profile while replacing to the current),
so that it references to the wrong memory location when doing dfa
lookups.

Replacement is not a common operation so make sure all memory
aliases are removed before the dfa goes live.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/match.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
Andy Whitcroft - April 13, 2010, 9:17 a.m.
On Tue, Apr 13, 2010 at 12:09:37AM -0700, john.johansen@canonical.com wrote:
> From: John Johansen <john.johansen@canonical.com>
> 
> OriginalAuthor: John Johansen <john.johansen@canonical.com>
> OriginalLocation: git://git.kernel.org/pub/scm/linux/kernel/git/jj/linux-apparm$
> commit: 6857acf643ba19eddfc29125fc011a3ce48fe87b
> BugLink: http://bugs.launchpad.net/bugs/529288
> 
> vmalloc doesn't guarentee that the tlbs of all cpus will be flushed
> when it completes.  Instead the tlbs gets flushed lazily, however for
> AppArmor this is a problem as the dfa becomes live to all cpus as
> soon as the profile replacedby value is written (this is even before
> locking of the lists are removed).
> 
> It is possible for another cpu to be in a state where it has an old
> tlb mapping for the vmalloc address (this will be caused by putting
> a reference on an old profile while replacing to the current),
> so that it references to the wrong memory location when doing dfa
> lookups.
> 
> Replacement is not a common operation so make sure all memory
> aliases are removed before the dfa goes live.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>
> ---
>  security/apparmor/match.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/security/apparmor/match.c b/security/apparmor/match.c
> index 5a55959..afc2dd2 100644
> --- a/security/apparmor/match.c
> +++ b/security/apparmor/match.c
> @@ -50,6 +50,7 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  {
>  	struct table_header *table = NULL;
>  	struct table_header th;
> +	int unmap_alias = 0;
>  	size_t tsize;
>  
>  	if (bsize < sizeof(struct table_header))
> @@ -73,8 +74,10 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  
>  	/* freed by free_table */
>  	table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
> -	if (!table)
> +	if (!table) {
> +		unmap_alias = 1;
>  		table = vmalloc(tsize);

It is possible this could be below more optimally:

		table = vmalloc(tsize);
		if (table)
			unmap_alias = 1

> +	}
>  	if (table) {
>  		*table = th;
>  		if (th.td_flags == YYTD_DATA8)
> @@ -91,6 +94,8 @@ static struct table_header *unpack_table(char *blob, size_t bsize)
>  	}
>  
>  out:
> +	if (unmap_alias)
> +		vm_unmap_aliases();
>  	return table;
>  fail:
>  	free_table(table);

Even without the above it seems sane and necessary:

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

Patch

diff --git a/security/apparmor/match.c b/security/apparmor/match.c
index 5a55959..afc2dd2 100644
--- a/security/apparmor/match.c
+++ b/security/apparmor/match.c
@@ -50,6 +50,7 @@  static struct table_header *unpack_table(char *blob, size_t bsize)
 {
 	struct table_header *table = NULL;
 	struct table_header th;
+	int unmap_alias = 0;
 	size_t tsize;
 
 	if (bsize < sizeof(struct table_header))
@@ -73,8 +74,10 @@  static struct table_header *unpack_table(char *blob, size_t bsize)
 
 	/* freed by free_table */
 	table = kmalloc(tsize, GFP_KERNEL | __GFP_NOWARN);
-	if (!table)
+	if (!table) {
+		unmap_alias = 1;
 		table = vmalloc(tsize);
+	}
 	if (table) {
 		*table = th;
 		if (th.td_flags == YYTD_DATA8)
@@ -91,6 +94,8 @@  static struct table_header *unpack_table(char *blob, size_t bsize)
 	}
 
 out:
+	if (unmap_alias)
+		vm_unmap_aliases();
 	return table;
 fail:
 	free_table(table);