[PATCH linux-next] SELinux: security_load_policy: Silence frame-larger-than warning

[PATCH linux-next] SELinux: security_load_policy: Silence frame-larger-than warning

[Tim Gardner]

Dynamically allocate a couple of the larger stack variables in order to
reduce the stack footprint below 1024. gcc-4.8

security/selinux/ss/services.c: In function 'security_load_policy':
security/selinux/ss/services.c:1964:1: warning: the frame size of 1104 bytes is larger than 1024 bytes [-Wframe-larger-than=]
 }

Also silence a couple of checkpatch warnings at the same time.

WARNING: sizeof policydb should be sizeof(policydb)
+	memcpy(oldpolicydb, &policydb, sizeof policydb);

WARNING: sizeof policydb should be sizeof(policydb)
+	memcpy(&policydb, newpolicydb, sizeof policydb);

Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Eric Paris <eparis@parisplace.org>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 security/selinux/ss/services.c |   54 ++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b4feecc..c8317c8 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1828,7 +1828,7 @@ static int security_preserve_bools(struct policydb *p);
  */
 int security_load_policy(void *data, size_t len)
 {
-	struct policydb oldpolicydb, newpolicydb;
+	struct policydb *oldpolicydb, *newpolicydb;
 	struct sidtab oldsidtab, newsidtab;
 	struct selinux_mapping *oldmap, *map = NULL;
 	struct convert_context_args args;
@@ -1837,12 +1837,19 @@ int security_load_policy(void *data, size_t len)
 	int rc = 0;
 	struct policy_file file = { data, len }, *fp = &file;
 
+	oldpolicydb = kzalloc(2 * sizeof(*oldpolicydb), GFP_KERNEL);
+	if (!oldpolicydb) {
+		rc = -ENOMEM;
+		goto out;
+	}
+	newpolicydb = oldpolicydb + 1;
+
 	if (!ss_initialized) {
 		avtab_cache_init();
 		rc = policydb_read(&policydb, fp);
 		if (rc) {
 			avtab_cache_destroy();
-			return rc;
+			goto out;
 		}
 
 		policydb.len = len;
@@ -1852,14 +1859,14 @@ int security_load_policy(void *data, size_t len)
 		if (rc) {
 			policydb_destroy(&policydb);
 			avtab_cache_destroy();
-			return rc;
+			goto out;
 		}
 
 		rc = policydb_load_isids(&policydb, &sidtab);
 		if (rc) {
 			policydb_destroy(&policydb);
 			avtab_cache_destroy();
-			return rc;
+			goto out;
 		}
 
 		security_load_policycaps();
@@ -1871,36 +1878,36 @@ int security_load_policy(void *data, size_t len)
 		selinux_status_update_policyload(seqno);
 		selinux_netlbl_cache_invalidate();
 		selinux_xfrm_notify_policyload();
-		return 0;
+		goto out;
 	}
 
 #if 0
 	sidtab_hash_eval(&sidtab, "sids");
 #endif
 
-	rc = policydb_read(&newpolicydb, fp);
+	rc = policydb_read(newpolicydb, fp);
 	if (rc)
-		return rc;
+		goto out;
 
-	newpolicydb.len = len;
+	newpolicydb->len = len;
 	/* If switching between different policy types, log MLS status */
-	if (policydb.mls_enabled && !newpolicydb.mls_enabled)
+	if (policydb.mls_enabled && !newpolicydb->mls_enabled)
 		printk(KERN_INFO "SELinux: Disabling MLS support...\n");
-	else if (!policydb.mls_enabled && newpolicydb.mls_enabled)
+	else if (!policydb.mls_enabled && newpolicydb->mls_enabled)
 		printk(KERN_INFO "SELinux: Enabling MLS support...\n");
 
-	rc = policydb_load_isids(&newpolicydb, &newsidtab);
+	rc = policydb_load_isids(newpolicydb, &newsidtab);
 	if (rc) {
 		printk(KERN_ERR "SELinux:  unable to load the initial SIDs\n");
-		policydb_destroy(&newpolicydb);
-		return rc;
+		policydb_destroy(newpolicydb);
+		goto out;
 	}
 
-	rc = selinux_set_mapping(&newpolicydb, secclass_map, &map, &map_size);
+	rc = selinux_set_mapping(newpolicydb, secclass_map, &map, &map_size);
 	if (rc)
 		goto err;
 
-	rc = security_preserve_bools(&newpolicydb);
+	rc = security_preserve_bools(newpolicydb);
 	if (rc) {
 		printk(KERN_ERR "SELinux:  unable to preserve booleans\n");
 		goto err;
@@ -1918,7 +1925,7 @@ int security_load_policy(void *data, size_t len)
 	 * in the new SID table.
 	 */
 	args.oldp = &policydb;
-	args.newp = &newpolicydb;
+	args.newp = newpolicydb;
 	rc = sidtab_map(&newsidtab, convert_context, &args);
 	if (rc) {
 		printk(KERN_ERR "SELinux:  unable to convert the internal"
@@ -1928,12 +1935,12 @@ int security_load_policy(void *data, size_t len)
 	}
 
 	/* Save the old policydb and SID table to free later. */
-	memcpy(&oldpolicydb, &policydb, sizeof policydb);
+	memcpy(oldpolicydb, &policydb, sizeof(policydb));
 	sidtab_set(&oldsidtab, &sidtab);
 
 	/* Install the new policydb and SID table. */
 	write_lock_irq(&policy_rwlock);
-	memcpy(&policydb, &newpolicydb, sizeof policydb);
+	memcpy(&policydb, newpolicydb, sizeof(policydb));
 	sidtab_set(&sidtab, &newsidtab);
 	security_load_policycaps();
 	oldmap = current_mapping;
@@ -1943,7 +1950,7 @@ int security_load_policy(void *data, size_t len)
 	write_unlock_irq(&policy_rwlock);
 
 	/* Free the old policydb and SID table. */
-	policydb_destroy(&oldpolicydb);
+	policydb_destroy(oldpolicydb);
 	sidtab_destroy(&oldsidtab);
 	kfree(oldmap);
 
@@ -1953,14 +1960,17 @@ int security_load_policy(void *data, size_t len)
 	selinux_netlbl_cache_invalidate();
 	selinux_xfrm_notify_policyload();
 
-	return 0;
+	rc = 0;
+	goto out;
 
 err:
 	kfree(map);
 	sidtab_destroy(&newsidtab);
-	policydb_destroy(&newpolicydb);
-	return rc;
+	policydb_destroy(newpolicydb);
 
+out:
+	kfree(oldpolicydb);
+	return rc;
 }
 
 size_t security_policydb_len(void)
-- 
1.7.9.5

Re: [PATCH linux-next] SELinux: security_load_policy: Silence frame-larger-than warning

[Paul Moore]

On Thursday, November 14, 2013 03:04:51 PM Tim Gardner wrote:
> Dynamically allocate a couple of the larger stack variables in order to
> reduce the stack footprint below 1024. gcc-4.8
> 
> security/selinux/ss/services.c: In function 'security_load_policy':
> security/selinux/ss/services.c:1964:1: warning: the frame size of 1104 bytes
> is larger than 1024 bytes [-Wframe-larger-than=] }
> 
> Also silence a couple of checkpatch warnings at the same time.
> 
> WARNING: sizeof policydb should be sizeof(policydb)
> +	memcpy(oldpolicydb, &policydb, sizeof policydb);
> 
> WARNING: sizeof policydb should be sizeof(policydb)
> +	memcpy(&policydb, newpolicydb, sizeof policydb);
> 
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Eric Paris <eparis@parisplace.org>
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  security/selinux/ss/services.c |   54 ++++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 22 deletions(-)

Applied, thanks.  It will be pushed to my next tree once -rc1 is released.

In the future, please send SELinux patches to the SELinux mailing list.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b4feecc..c8317c8 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1828,7 +1828,7 @@ static int security_preserve_bools(struct policydb
> *p); */
>  int security_load_policy(void *data, size_t len)
>  {
> -	struct policydb oldpolicydb, newpolicydb;
> +	struct policydb *oldpolicydb, *newpolicydb;
>  	struct sidtab oldsidtab, newsidtab;
>  	struct selinux_mapping *oldmap, *map = NULL;
>  	struct convert_context_args args;
> @@ -1837,12 +1837,19 @@ int security_load_policy(void *data, size_t len)
>  	int rc = 0;
>  	struct policy_file file = { data, len }, *fp = &file;
> 
> +	oldpolicydb = kzalloc(2 * sizeof(*oldpolicydb), GFP_KERNEL);
> +	if (!oldpolicydb) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +	newpolicydb = oldpolicydb + 1;
> +
>  	if (!ss_initialized) {
>  		avtab_cache_init();
>  		rc = policydb_read(&policydb, fp);
>  		if (rc) {
>  			avtab_cache_destroy();
> -			return rc;
> +			goto out;
>  		}
> 
>  		policydb.len = len;
> @@ -1852,14 +1859,14 @@ int security_load_policy(void *data, size_t len)
>  		if (rc) {
>  			policydb_destroy(&policydb);
>  			avtab_cache_destroy();
> -			return rc;
> +			goto out;
>  		}
> 
>  		rc = policydb_load_isids(&policydb, &sidtab);
>  		if (rc) {
>  			policydb_destroy(&policydb);
>  			avtab_cache_destroy();
> -			return rc;
> +			goto out;
>  		}
> 
>  		security_load_policycaps();
> @@ -1871,36 +1878,36 @@ int security_load_policy(void *data, size_t len)
>  		selinux_status_update_policyload(seqno);
>  		selinux_netlbl_cache_invalidate();
>  		selinux_xfrm_notify_policyload();
> -		return 0;
> +		goto out;
>  	}
> 
>  #if 0
>  	sidtab_hash_eval(&sidtab, "sids");
>  #endif
> 
> -	rc = policydb_read(&newpolicydb, fp);
> +	rc = policydb_read(newpolicydb, fp);
>  	if (rc)
> -		return rc;
> +		goto out;
> 
> -	newpolicydb.len = len;
> +	newpolicydb->len = len;
>  	/* If switching between different policy types, log MLS status */
> -	if (policydb.mls_enabled && !newpolicydb.mls_enabled)
> +	if (policydb.mls_enabled && !newpolicydb->mls_enabled)
>  		printk(KERN_INFO "SELinux: Disabling MLS support...\n");
> -	else if (!policydb.mls_enabled && newpolicydb.mls_enabled)
> +	else if (!policydb.mls_enabled && newpolicydb->mls_enabled)
>  		printk(KERN_INFO "SELinux: Enabling MLS support...\n");
> 
> -	rc = policydb_load_isids(&newpolicydb, &newsidtab);
> +	rc = policydb_load_isids(newpolicydb, &newsidtab);
>  	if (rc) {
>  		printk(KERN_ERR "SELinux:  unable to load the initial SIDs\n");
> -		policydb_destroy(&newpolicydb);
> -		return rc;
> +		policydb_destroy(newpolicydb);
> +		goto out;
>  	}
> 
> -	rc = selinux_set_mapping(&newpolicydb, secclass_map, &map, &map_size);
> +	rc = selinux_set_mapping(newpolicydb, secclass_map, &map, &map_size);
>  	if (rc)
>  		goto err;
> 
> -	rc = security_preserve_bools(&newpolicydb);
> +	rc = security_preserve_bools(newpolicydb);
>  	if (rc) {
>  		printk(KERN_ERR "SELinux:  unable to preserve booleans\n");
>  		goto err;
> @@ -1918,7 +1925,7 @@ int security_load_policy(void *data, size_t len)
>  	 * in the new SID table.
>  	 */
>  	args.oldp = &policydb;
> -	args.newp = &newpolicydb;
> +	args.newp = newpolicydb;
>  	rc = sidtab_map(&newsidtab, convert_context, &args);
>  	if (rc) {
>  		printk(KERN_ERR "SELinux:  unable to convert the internal"
> @@ -1928,12 +1935,12 @@ int security_load_policy(void *data, size_t len)
>  	}
> 
>  	/* Save the old policydb and SID table to free later. */
> -	memcpy(&oldpolicydb, &policydb, sizeof policydb);
> +	memcpy(oldpolicydb, &policydb, sizeof(policydb));
>  	sidtab_set(&oldsidtab, &sidtab);
> 
>  	/* Install the new policydb and SID table. */
>  	write_lock_irq(&policy_rwlock);
> -	memcpy(&policydb, &newpolicydb, sizeof policydb);
> +	memcpy(&policydb, newpolicydb, sizeof(policydb));
>  	sidtab_set(&sidtab, &newsidtab);
>  	security_load_policycaps();
>  	oldmap = current_mapping;
> @@ -1943,7 +1950,7 @@ int security_load_policy(void *data, size_t len)
>  	write_unlock_irq(&policy_rwlock);
> 
>  	/* Free the old policydb and SID table. */
> -	policydb_destroy(&oldpolicydb);
> +	policydb_destroy(oldpolicydb);
>  	sidtab_destroy(&oldsidtab);
>  	kfree(oldmap);
> 
> @@ -1953,14 +1960,17 @@ int security_load_policy(void *data, size_t len)
>  	selinux_netlbl_cache_invalidate();
>  	selinux_xfrm_notify_policyload();
> 
> -	return 0;
> +	rc = 0;
> +	goto out;
> 
>  err:
>  	kfree(map);
>  	sidtab_destroy(&newsidtab);
> -	policydb_destroy(&newpolicydb);
> -	return rc;
> +	policydb_destroy(newpolicydb);
> 
> +out:
> +	kfree(oldpolicydb);
> +	return rc;
>  }
> 
>  size_t security_policydb_len(void)

-- 
paul moore
www.paul-moore.com

Re: [PATCH linux-next] SELinux: security_load_policy: Silence frame-larger-than warning

[Tim Gardner]

On 11/19/2013 02:38 PM, Paul Moore wrote:
> On Thursday, November 14, 2013 03:04:51 PM Tim Gardner wrote:
>> Dynamically allocate a couple of the larger stack variables in order to
>> reduce the stack footprint below 1024. gcc-4.8
>>
>> security/selinux/ss/services.c: In function 'security_load_policy':
>> security/selinux/ss/services.c:1964:1: warning: the frame size of 1104 bytes
>> is larger than 1024 bytes [-Wframe-larger-than=] }
>>
>> Also silence a couple of checkpatch warnings at the same time.
>>
>> WARNING: sizeof policydb should be sizeof(policydb)
>> +	memcpy(oldpolicydb, &policydb, sizeof policydb);
>>
>> WARNING: sizeof policydb should be sizeof(policydb)
>> +	memcpy(&policydb, newpolicydb, sizeof policydb);
>>
>> Cc: Stephen Smalley <sds@tycho.nsa.gov>
>> Cc: James Morris <james.l.morris@oracle.com>
>> Cc: Eric Paris <eparis@parisplace.org>
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>> ---
>>  security/selinux/ss/services.c |   54 ++++++++++++++++++++++--------------
>>  1 file changed, 32 insertions(+), 22 deletions(-)
> 
> Applied, thanks.  It will be pushed to my next tree once -rc1 is released.
> 
> In the future, please send SELinux patches to the SELinux mailing list.
> 

It is difficult to know where to send a patch for every subsystem. I've
been using the get_maintainer.pl script, so perhaps the info in
MAINTAINERS is stale ? I'm open to suggestions.

$: scripts/get_maintainer.pl -f security/selinux/ss/services.c
Stephen Smalley <sds@tycho.nsa.gov> (supporter:SELINUX SECURITY...)
James Morris <james.l.morris@oracle.com> (supporter:SELINUX SECURITY...)
Eric Paris <eparis@parisplace.org> (supporter:SELINUX SECURITY...)
linux-security-module@vger.kernel.org (open list:SECURITY SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)

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

Re: [PATCH linux-next] SELinux: security_load_policy: Silence frame-larger-than warning

[Paul Moore]

On Tuesday, November 19, 2013 02:46:12 PM Tim Gardner wrote:
> On 11/19/2013 02:38 PM, Paul Moore wrote:
> > On Thursday, November 14, 2013 03:04:51 PM Tim Gardner wrote:
> >> Dynamically allocate a couple of the larger stack variables in order to
> >> reduce the stack footprint below 1024. gcc-4.8
> >> 
> >> security/selinux/ss/services.c: In function 'security_load_policy':
> >> security/selinux/ss/services.c:1964:1: warning: the frame size of 1104
> >> bytes is larger than 1024 bytes [-Wframe-larger-than=] }
> >> 
> >> Also silence a couple of checkpatch warnings at the same time.
> >> 
> >> WARNING: sizeof policydb should be sizeof(policydb)
> >> +	memcpy(oldpolicydb, &policydb, sizeof policydb);
> >> 
> >> WARNING: sizeof policydb should be sizeof(policydb)
> >> +	memcpy(&policydb, newpolicydb, sizeof policydb);
> >> 
> >> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> >> Cc: James Morris <james.l.morris@oracle.com>
> >> Cc: Eric Paris <eparis@parisplace.org>
> >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> >> ---
> >> 
> >>  security/selinux/ss/services.c |   54
> >>  ++++++++++++++++++++++--------------
> >>  1 file changed, 32 insertions(+), 22 deletions(-)
> > 
> > Applied, thanks.  It will be pushed to my next tree once -rc1 is released.
> > 
> > In the future, please send SELinux patches to the SELinux mailing list.
> 
> It is difficult to know where to send a patch for every subsystem. I've
> been using the get_maintainer.pl script, so perhaps the info in
> MAINTAINERS is stale ? I'm open to suggestions.

The SELinux mailing list information in the MAINTAINERS file isn't stale, 
although we have just recently changed the tree location and that patch hasn't 
hit Linus' tree yet.  Here is the most current snippet:

 SELINUX SECURITY MODULE
 M:      Stephen Smalley <sds@tycho.nsa.gov>
 M:      James Morris <james.l.morris@oracle.com>
 M:      Eric Paris <eparis@parisplace.org>
 M:      Paul Moore <paul@paul-moore.com>
 L:      selinux@tycho.nsa.gov (subscribers-only, general discussion)
 W:      http://selinuxproject.org
 T:      git git://git.infradead.org/users/pcmoore/selinux
 S:      Supported
 F:      include/linux/selinux*
 F:      security/selinux/
 F:      scripts/selinux/

My Perl is a bit rusty/non-existent so I'm not sure why the get_maintainer 
script isn't showing the mailing list information, so in the meantime I would 
suggest just looking through the file by hand.
 
> $: scripts/get_maintainer.pl -f security/selinux/ss/services.c
> Stephen Smalley <sds@tycho.nsa.gov> (supporter:SELINUX SECURITY...)
> James Morris <james.l.morris@oracle.com> (supporter:SELINUX SECURITY...)
> Eric Paris <eparis@parisplace.org> (supporter:SELINUX SECURITY...)
> linux-security-module@vger.kernel.org (open list:SECURITY SUBSYSTEM)
> linux-kernel@vger.kernel.org (open list)
> 
> rtg

-- 
paul moore
www.paul-moore.com