[RFC 0/5] Optimizations for memory handling in smk_write_rules_list()

[RFC 0/5] Optimizations for memory handling in smk_write_rules_list()

[Tomasz Stanislawski]

Hi everyone,
This patchset focuses on optimizations for memory handling done in
smk_write_rules_list().  Basically, all the patches try to avoid calling
kmalloc().  I expect that there might be some controversy about these patches.

The first patch introduces a limit for length if SMACK rules in 'long' format.
The limit was chosen to handle two long labels (SMK_LONGLABEL) plus access
flags plus two separators.  Passing a longer rule is likely an error due to too
long labels or having a rule string that contains a ridiculous amount of
whitespaces.  Introducing the limit helps to protect the kernel if userspace
(maybe privileged but still userspace) passed too large write(). Calling
kmalloc() for a large size wull trigger a significant number of page reclaim
actions or even OOM.  Moreover, such an allocation is very likely to fail and
it should be avoided. Calling vmalloc() would be a much safer idea.  Anyway,
the buffer is valid only during smk_write_rules_list() therefore allocating a
buffer on stack simplifies memory management.

The second patch simplifies memory management in smk_parse_long_rule() and
speeds up parsing.

The third patch fixes a potential memory leak in smk_write_rules_list().  I was
not able to find any reference to smack_parsed_rule after the function is
finished. Notice that smk_user_access() does not use dynamic any allocation for
smack_parsed_rule.  I kindly ask you to review the patch.

The last two patches uses kmem_cache to avoid wasting memory for padding bytes
for the most frequently allocated structures in SMACK.

The patchset provides a small but observable performance improvement of SMACK
initialization phase.  The results below were collected for 17K rules and 500
labels using command 'time smackctl apply'.

        |  Run1 |  Run2 |  Run3 |  Run4 |  Run5 |  Avg  | Gain [ms]
--------------------------------------------------------------
Ref     | 1.177 | 1.174 | 1.177 | 1.179 | 1.171 | 1.176 | 0
patch1  | 1.165 | 1.177 | 1.178 | 1.167 | 1.175 | 1.172 | 3
patch2  | 1.145 | 1.142 | 1.140 | 1.139 | 1.137 | 1.141 | 35
patch3  | 1.117 | 1.128 | 1.126 | 1.129 | 1.124 | 1.125 | 51
patch4  | 1.097 | 1.120 | 1.119 | 1.112 | 1.120 | 1.114 | 62
patch5  | 1.092 | 1.102 | 1.090 | 1.108 | 1.091 | 1.097 | 79

Every patch introduces a tiny bit of speed improvement.  Total gain is 7%.
It will increase to 15% if the patch for label hashing [1] is applied.

I hope you find this patchset useful.
Regards,
Tomasz Stanislawski

References:
[1] http://thread.gmane.org/gmane.linux.kernel.lsm/19609


Tomasz Stanislawski (5):
  security: smack: avoid kmalloc allocations while loading a rule
    string
  security: smack: avoid kmalloc() in smk_parse_long_rule()
  security: smack: fix memleak in smk_write_rules_list()
  security: smack: add kmem_cache for smack_rule allocations
  security: smack: add kmem_cache for smack_master_list allocations

 security/smack/smack.h     |   10 ++++
 security/smack/smack_lsm.c |   19 ++++++-
 security/smack/smackfs.c   |  122 +++++++++++++++++++-------------------------
 3 files changed, 80 insertions(+), 71 deletions(-)

-- 
1.7.9.5

[RFC 1/5] security: smack: avoid kmalloc allocations while loading a rule string

[Tomasz Stanislawski]

The maximal length for a rule line for long format is introduced as
SMK_LOAD2LEN. This allows a buffer for a rule string to be allocated
on a stack instead of a heap (aka kmalloc cache).

Limiting the length of a rule line helps to avoid allocations of a very long
contiguous buffer from a heap if user calls write() for a very long chunk.
Such an allocation often causes a lot swapper/writeback havoc and it is very
likely to fails.

Moreover, stack allocation is slightly faster than from kmalloc.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 security/smack/smackfs.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 53a08b8..9a3cd0d 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -137,6 +137,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
  * SMK_ACCESS: Maximum possible combination of access permissions
  * SMK_ACCESSLEN: Maximum length for a rule access field
  * SMK_LOADLEN: Smack rule length
+ * SMK_LOAD2LEN: Smack maximal long rule length excluding \0
  */
 #define SMK_OACCESS	"rwxa"
 #define SMK_ACCESS	"rwxat"
@@ -144,6 +145,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
 #define SMK_ACCESSLEN	(sizeof(SMK_ACCESS) - 1)
 #define SMK_OLOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_OACCESSLEN)
 #define SMK_LOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_ACCESSLEN)
+#define SMK_LOAD2LEN	(2 * SMK_LONGLABEL + SMK_ACCESSLEN + 2)
 
 /*
  * Stricly for CIPSO level manipulation.
@@ -447,8 +449,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 {
 	struct smack_known *skp;
 	struct smack_parsed_rule *rule;
-	char *data;
-	int datalen;
+	char data[SMK_LOAD2LEN + 1];
 	int rc = -EINVAL;
 	int load = 0;
 
@@ -465,13 +466,10 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 		 */
 		if (count != SMK_OLOADLEN && count != SMK_LOADLEN)
 			return -EINVAL;
-		datalen = SMK_LOADLEN;
-	} else
-		datalen = count + 1;
+	}
 
-	data = kzalloc(datalen, GFP_KERNEL);
-	if (data == NULL)
-		return -ENOMEM;
+	if (count > SMK_LOAD2LEN)
+		count = SMK_LOAD2LEN;
 
 	if (copy_from_user(data, buf, count) != 0) {
 		rc = -EFAULT;
@@ -522,7 +520,6 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 out_free_rule:
 	kfree(rule);
 out:
-	kfree(data);
 	return rc;
 }
 
-- 
1.7.9.5

[RFC 2/5] security: smack: avoid kmalloc() in smk_parse_long_rule()

[Tomasz Stanislawski]

Function smk_parse_long_rule() allocates a number of temporary strings on heap
(kmalloc cache). Moreover, the sizes of those allocations might be large if
user calls write() for a long chunk. A big kmalloc triggers a heavy reclaim
havoc and it is very likely to fail.

This patch introduces smk_parse_substrings() function that parses a string into
substring separated by whitespaces.  The buffer for substring is preallocated.
It must store substring the worst case scenario which is SMK_LOAD2LEN in case
of long rule parsing.

The buffer is allocated on stack what is slightly faster than kmalloc().

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 security/smack/smackfs.c |   67 +++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 9a3cd0d..46f111e 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -364,6 +364,33 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
 }
 
 /**
+ * smk_parse_strings - parse white-space separated substring from a string
+ * @src: a long string to be parsed, null terminated
+ * @dst: a buffer for substrings, should be at least strlen(src)+1 bytes
+ * @args: table for parsed substring
+ * @size: number of slots in args table
+ *
+ * Returns number of parsed substrings
+ */
+static int smk_parse_substrings(const char *src, char *dst,
+	char *args[], int size)
+{
+	int cnt;
+
+	for (cnt = 0; cnt < size; ++cnt) {
+		src = skip_spaces(src);
+		if (*src == 0)
+			break;
+		args[cnt] = dst;
+		for (; *src && !isspace(*src); ++src, ++dst)
+			*dst = *src;
+		*(dst++) = 0;
+	}
+
+	return cnt;
+}
+
+/**
  * smk_parse_long_rule - parse Smack rule from rule string
  * @data: string to be parsed, null terminated
  * @rule: Will be filled with Smack parsed rule
@@ -375,48 +402,20 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
 static int smk_parse_long_rule(const char *data, struct smack_parsed_rule *rule,
 				int import, int change)
 {
-	char *subject;
-	char *object;
-	char *access1;
-	char *access2;
-	int datalen;
+	char tmp[SMK_LOAD2LEN + 1];
+	char *args[4];
 	int rc = -1;
 
-	/* This is inefficient */
-	datalen = strlen(data);
-
-	/* Our first element can be 64 + \0 with no spaces */
-	subject = kzalloc(datalen + 1, GFP_KERNEL);
-	if (subject == NULL)
-		return -1;
-	object = kzalloc(datalen, GFP_KERNEL);
-	if (object == NULL)
-		goto free_out_s;
-	access1 = kzalloc(datalen, GFP_KERNEL);
-	if (access1 == NULL)
-		goto free_out_o;
-	access2 = kzalloc(datalen, GFP_KERNEL);
-	if (access2 == NULL)
-		goto free_out_a;
-
 	if (change) {
-		if (sscanf(data, "%s %s %s %s",
-			subject, object, access1, access2) == 4)
-			rc = smk_fill_rule(subject, object, access1, access2,
+		if (smk_parse_substrings(data, tmp, args, 4) == 4)
+			rc = smk_fill_rule(args[0], args[1], args[2], args[3],
 				rule, import, 0);
 	} else {
-		if (sscanf(data, "%s %s %s", subject, object, access1) == 3)
-			rc = smk_fill_rule(subject, object, access1, NULL,
+		if (smk_parse_substrings(data, tmp, args, 3) == 3)
+			rc = smk_fill_rule(args[0], args[1], args[2], NULL,
 				rule, import, 0);
 	}
 
-	kfree(access2);
-free_out_a:
-	kfree(access1);
-free_out_o:
-	kfree(object);
-free_out_s:
-	kfree(subject);
 	return rc;
 }
 
-- 
1.7.9.5

[RFC 3/5] security: smack: fix memleak in smk_write_rules_list()

[Tomasz Stanislawski]

The smack_parsed_rule structure is allocated.  If a rule is successfully
installed then the last reference to the object is lost.  This patch fixes this
leak. Moreover smack_parsed_rule is allocated on stack because it no longer
needed ofter smk_write_rules_list() is finished.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 46f111e..e8c57f3 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -447,7 +447,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 					struct mutex *rule_lock, int format)
 {
 	struct smack_known *skp;
-	struct smack_parsed_rule *rule;
+	struct smack_parsed_rule rule;
 	char data[SMK_LOAD2LEN + 1];
 	int rc = -EINVAL;
 	int load = 0;
@@ -475,49 +475,39 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 		goto out;
 	}
 
-	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
-	if (rule == NULL) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
 	if (format == SMK_LONG_FMT) {
 		/*
 		 * Be sure the data string is terminated.
 		 */
 		data[count] = '\0';
-		if (smk_parse_long_rule(data, rule, 1, 0))
-			goto out_free_rule;
+		if (smk_parse_long_rule(data, &rule, 1, 0))
+			goto out;
 	} else if (format == SMK_CHANGE_FMT) {
 		data[count] = '\0';
-		if (smk_parse_long_rule(data, rule, 1, 1))
-			goto out_free_rule;
+		if (smk_parse_long_rule(data, &rule, 1, 1))
+			goto out;
 	} else {
 		/*
 		 * More on the minor hack for backward compatibility
 		 */
 		if (count == (SMK_OLOADLEN))
 			data[SMK_OLOADLEN] = '-';
-		if (smk_parse_rule(data, rule, 1))
-			goto out_free_rule;
+		if (smk_parse_rule(data, &rule, 1))
+			goto out;
 	}
 
 
 	if (rule_list == NULL) {
 		load = 1;
-		skp = smk_find_entry(rule->smk_subject);
+		skp = smk_find_entry(rule.smk_subject);
 		rule_list = &skp->smk_rules;
 		rule_lock = &skp->smk_rules_lock;
 	}
 
-	rc = smk_set_access(rule, rule_list, rule_lock, load);
-	if (rc == 0) {
+	rc = smk_set_access(&rule, rule_list, rule_lock, load);
+	if (rc == 0)
 		rc = count;
-		goto out;
-	}
 
-out_free_rule:
-	kfree(rule);
 out:
 	return rc;
 }
-- 
1.7.9.5

[RFC 4/5] security: smack: add kmem_cache for smack_rule allocations

[Tomasz Stanislawski]

On ARM, sizeof(struct smack_rule)==20. Allocation by kmalloc() uses a
32-byte-long chunk to allocate 20 bytes. Just ask ksize().  It means that 40%
of memory is simply wasted for padding bytes.

The problem is fixed in this patch by using kmem_cache. The cache allocates
struct smack_rule using 24-byte-long chunks according to ksize(). This reduces
amount of used memory by 25%.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 security/smack/smack.h     |    3 +++
 security/smack/smack_lsm.c |   11 ++++++++++-
 security/smack/smackfs.c   |    2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 8ad3095..38ba673 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -233,6 +233,9 @@ extern struct mutex	smack_known_lock;
 extern struct list_head smack_known_list;
 extern struct list_head smk_netlbladdr_list;
 
+/* Cache for fast and thrifty allocations */
+extern struct kmem_cache *smack_rule_cache;
+
 extern struct security_operations smack_ops;
 
 /*
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index d52c780..7aa696a 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3564,6 +3564,9 @@ static __init void init_smack_known_list(void)
 	list_add(&smack_known_web.list, &smack_known_list);
 }
 
+/* KMEM caches for fast and thrifty allocations */
+struct kmem_cache *smack_rule_cache;
+
 /**
  * smack_init - initialize the smack system
  *
@@ -3577,10 +3580,16 @@ static __init int smack_init(void)
 	if (!security_module_enable(&smack_ops))
 		return 0;
 
+	smack_rule_cache = KMEM_CACHE(smack_rule, 0);
+	if (!smack_rule_cache)
+		return -ENOMEM;
+
 	tsp = new_task_smack(smack_known_floor.smk_known,
 				smack_known_floor.smk_known, GFP_KERNEL);
-	if (tsp == NULL)
+	if (tsp == NULL) {
+		kmem_cache_destroy(smack_rule_cache);
 		return -ENOMEM;
+	}
 
 	printk(KERN_INFO "Smack:  Initializing.\n");
 
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index e8c57f3..c08b1ec 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -217,7 +217,7 @@ static int smk_set_access(struct smack_parsed_rule *srp,
 	}
 
 	if (found == 0) {
-		sp = kzalloc(sizeof(*sp), GFP_KERNEL);
+		sp = kmem_cache_zalloc(smack_rule_cache, GFP_KERNEL);
 		if (sp == NULL) {
 			rc = -ENOMEM;
 			goto out;
-- 
1.7.9.5

[RFC 5/5] security: smack: add kmem_cache for smack_master_list allocations

[Tomasz Stanislawski]

On ARM, sizeof(struct smack_master_list) == 12. Allocation by kmalloc() uses a
32-byte-long chunk to allocate 12 bytes. Just ask ksize().  It means that 63%
of memory is simply wasted for padding bytes.

The problem is fixed in this patch by using kmem_cache. The cache allocates
struct smack_master_list using 16-byte-long chunks according to ksize(). This
reduces amount of used memory by 50%.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 security/smack/smack.h     |    7 +++++++
 security/smack/smack_lsm.c |    8 ++++++++
 security/smack/smackfs.c   |    8 ++------
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 38ba673..463f818 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -194,6 +194,12 @@ struct smk_audit_info {
 	struct smack_audit_data sad;
 #endif
 };
+
+struct smack_master_list {
+	struct list_head	list;
+	struct smack_rule	*smk_rule;
+};
+
 /*
  * These functions are in smack_lsm.c
  */
@@ -235,6 +241,7 @@ extern struct list_head smk_netlbladdr_list;
 
 /* Cache for fast and thrifty allocations */
 extern struct kmem_cache *smack_rule_cache;
+extern struct kmem_cache *smack_master_list_cache;
 
 extern struct security_operations smack_ops;
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7aa696a..1d4a1b0 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3566,6 +3566,7 @@ static __init void init_smack_known_list(void)
 
 /* KMEM caches for fast and thrifty allocations */
 struct kmem_cache *smack_rule_cache;
+struct kmem_cache *smack_master_list_cache;
 
 /**
  * smack_init - initialize the smack system
@@ -3584,9 +3585,16 @@ static __init int smack_init(void)
 	if (!smack_rule_cache)
 		return -ENOMEM;
 
+	smack_master_list_cache = KMEM_CACHE(smack_master_list, 0);
+	if (!smack_master_list_cache) {
+		kmem_cache_destroy(smack_rule_cache);
+		return -ENOMEM;
+	}
+
 	tsp = new_task_smack(smack_known_floor.smk_known,
 				smack_known_floor.smk_known, GFP_KERNEL);
 	if (tsp == NULL) {
+		kmem_cache_destroy(smack_master_list_cache);
 		kmem_cache_destroy(smack_rule_cache);
 		return -ENOMEM;
 	}
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index c08b1ec..c7a1b0d 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -104,11 +104,6 @@ LIST_HEAD(smk_netlbladdr_list);
  * Rule lists are maintained for each label.
  * This master list is just for reading /smack/load and /smack/load2.
  */
-struct smack_master_list {
-	struct list_head	list;
-	struct smack_rule	*smk_rule;
-};
-
 LIST_HEAD(smack_rule_list);
 
 struct smack_parsed_rule {
@@ -233,7 +228,8 @@ static int smk_set_access(struct smack_parsed_rule *srp,
 		 * it needs to get added for reporting.
 		 */
 		if (global) {
-			smlp = kzalloc(sizeof(*smlp), GFP_KERNEL);
+			smlp = kmem_cache_zalloc(smack_master_list_cache,
+							GFP_KERNEL);
 			if (smlp != NULL) {
 				smlp->smk_rule = sp;
 				list_add_rcu(&smlp->list, &smack_rule_list);
-- 
1.7.9.5

Re: [RFC 1/5] security: smack: avoid kmalloc allocations while loading a rule string

[Casey Schaufler]

On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
> The maximal length for a rule line for long format is introduced as
> SMK_LOAD2LEN. This allows a buffer for a rule string to be allocated
> on a stack instead of a heap (aka kmalloc cache).
>
> Limiting the length of a rule line helps to avoid allocations of a very long
> contiguous buffer from a heap if user calls write() for a very long chunk.
> Such an allocation often causes a lot swapper/writeback havoc and it is very
> likely to fails.
>
> Moreover, stack allocation is slightly faster than from kmalloc.
>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>

Please see the explanation below.

Nacked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/smack/smackfs.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 53a08b8..9a3cd0d 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -137,6 +137,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
>   * SMK_ACCESS: Maximum possible combination of access permissions
>   * SMK_ACCESSLEN: Maximum length for a rule access field
>   * SMK_LOADLEN: Smack rule length
> + * SMK_LOAD2LEN: Smack maximal long rule length excluding \0
>   */
>  #define SMK_OACCESS	"rwxa"
>  #define SMK_ACCESS	"rwxat"
> @@ -144,6 +145,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
>  #define SMK_ACCESSLEN	(sizeof(SMK_ACCESS) - 1)
>  #define SMK_OLOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_OACCESSLEN)
>  #define SMK_LOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_ACCESSLEN)
> +#define SMK_LOAD2LEN	(2 * SMK_LONGLABEL + SMK_ACCESSLEN + 2)
>  
>  /*
>   * Stricly for CIPSO level manipulation.
> @@ -447,8 +449,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  {
>  	struct smack_known *skp;
>  	struct smack_parsed_rule *rule;
> -	char *data;
> -	int datalen;
> +	char data[SMK_LOAD2LEN + 1];

That puts over 512 bytes on the stack. The reason that the code
uses a temporary allocation is that 512 bytes to considerably
beyond what is considered reasonable to put on the kernel stack.
As reasonable as this approach is in user space code, it is not
appropriate in the kernel.

>  	int rc = -EINVAL;
>  	int load = 0;
>  
> @@ -465,13 +466,10 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  		 */
>  		if (count != SMK_OLOADLEN && count != SMK_LOADLEN)
>  			return -EINVAL;
> -		datalen = SMK_LOADLEN;
> -	} else
> -		datalen = count + 1;
> +	}
>  
> -	data = kzalloc(datalen, GFP_KERNEL);
> -	if (data == NULL)
> -		return -ENOMEM;
> +	if (count > SMK_LOAD2LEN)
> +		count = SMK_LOAD2LEN;
>  
>  	if (copy_from_user(data, buf, count) != 0) {
>  		rc = -EFAULT;
> @@ -522,7 +520,6 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  out_free_rule:
>  	kfree(rule);
>  out:
> -	kfree(data);
>  	return rc;
>  }
>  

Re: [RFC 2/5] security: smack: avoid kmalloc() in smk_parse_long_rule()

[Casey Schaufler]

On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
> Function smk_parse_long_rule() allocates a number of temporary strings on heap
> (kmalloc cache). Moreover, the sizes of those allocations might be large if
> user calls write() for a long chunk. A big kmalloc triggers a heavy reclaim
> havoc and it is very likely to fail.
>
> This patch introduces smk_parse_substrings() function that parses a string into
> substring separated by whitespaces.  The buffer for substring is preallocated.
> It must store substring the worst case scenario which is SMK_LOAD2LEN in case
> of long rule parsing.
>
> The buffer is allocated on stack what is slightly faster than kmalloc().
>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>

There is hope for this patch, but it will need changes.

> ---
>  security/smack/smackfs.c |   67 +++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 34 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 9a3cd0d..46f111e 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -364,6 +364,33 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
>  }
>  
>  /**
> + * smk_parse_strings - parse white-space separated substring from a string
> + * @src: a long string to be parsed, null terminated
> + * @dst: a buffer for substrings, should be at least strlen(src)+1 bytes
> + * @args: table for parsed substring
> + * @size: number of slots in args table
> + *
> + * Returns number of parsed substrings
> + */
> +static int smk_parse_substrings(const char *src, char *dst,
> +	char *args[], int size)
> +{
> +	int cnt;
> +
> +	for (cnt = 0; cnt < size; ++cnt) {
> +		src = skip_spaces(src);
> +		if (*src == 0)
> +			break;
> +		args[cnt] = dst;
> +		for (; *src && !isspace(*src); ++src, ++dst)
> +			*dst = *src;
> +		*(dst++) = 0;
> +	}
> +
> +	return cnt;
> +}
> +
> +/**
>   * smk_parse_long_rule - parse Smack rule from rule string
>   * @data: string to be parsed, null terminated
>   * @rule: Will be filled with Smack parsed rule
> @@ -375,48 +402,20 @@ static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule,
>  static int smk_parse_long_rule(const char *data, astruct smack_parsed_rule *rule,
>  				int import, int change)
>  {
> -	char *subject;
> -	char *object;
> -	char *access1;
> -	char *access2;
> -	int datalen;
> +	char tmp[SMK_LOAD2LEN + 1];

As mentioned in patch 1 of this set, you can't put something this
large on the stack. You could however use the same logic below on
a single allocated buffer and reduce the number of kzallocs from
four to one. That would get most of the improvement you're looking
for.

> +	char *args[4];
>  	int rc = -1;
>  
> -	/* This is inefficient */
> -	datalen = strlen(data);
> -
> -	/* Our first element can be 64 + \0 with no spaces */
> -	subject = kzalloc(datalen + 1, GFP_KERNEL);
> -	if (subject == NULL)
> -		return -1;
> -	object = kzalloc(datalen, GFP_KERNEL);
> -	if (object == NULL)
> -		goto free_out_s;
> -	access1 = kzalloc(datalen, GFP_KERNEL);
> -	if (access1 == NULL)
> -		goto free_out_o;
> -	access2 = kzalloc(datalen, GFP_KERNEL);
> -	if (access2 == NULL)
> -		goto free_out_a;
> -
>  	if (change) {
> -		if (sscanf(data, "%s %s %s %s",
> -			subject, object, access1, access2) == 4)
> -			rc = smk_fill_rule(subject, object, access1, access2,
> +		if (smk_parse_substrings(data, tmp, args, 4) == 4)
> +			rc = smk_fill_rule(args[0], args[1], args[2], args[3],
>  				rule, import, 0);
>  	} else {
> -		if (sscanf(data, "%s %s %s", subject, object, access1) == 3)
> -			rc = smk_fill_rule(subject, object, access1, NULL,
> +		if (smk_parse_substrings(data, tmp, args, 3) == 3)
> +			rc = smk_fill_rule(args[0], args[1], args[2], NULL,
>  				rule, import, 0);
>  	}
>  
> -	kfree(access2);
> -free_out_a:
> -	kfree(access1);
> -free_out_o:
> -	kfree(object);
> -free_out_s:
> -	kfree(subject);
>  	return rc;
>  }
>  

Re: [RFC 3/5] security: smack: fix memleak in smk_write_rules_list()

[Casey Schaufler]

On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
> The smack_parsed_rule structure is allocated.  If a rule is successfully
> installed then the last reference to the object is lost.  This patch fixes this
> leak. Moreover smack_parsed_rule is allocated on stack because it no longer
> needed ofter smk_write_rules_list() is finished.
>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>

It looks like this was introduced with the change-rule support.
Prior to that, the rule passed into smk_set_access() was added
to the rule list if it was new. The change-rule support added a
new structure and missed the fact the rule was already allocated.

The patch needs to be rebased so that it does not depend on the
changes from patches 1 and 2 of the set.


> ---
>  security/smack/smackfs.c |   30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 46f111e..e8c57f3 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -447,7 +447,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  					struct mutex *rule_lock, int format)
>  {
>  	struct smack_known *skp;
> -	struct smack_parsed_rule *rule;
> +	struct smack_parsed_rule rule;
>  	char data[SMK_LOAD2LEN + 1];
>  	int rc = -EINVAL;
>  	int load = 0;
> @@ -475,49 +475,39 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  		goto out;
>  	}
>  
> -	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> -	if (rule == NULL) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> -
>  	if (format == SMK_LONG_FMT) {
>  		/*
>  		 * Be sure the data string is terminated.
>  		 */
>  		data[count] = '\0';
> -		if (smk_parse_long_rule(data, rule, 1, 0))
> -			goto out_free_rule;
> +		if (smk_parse_long_rule(data, &rule, 1, 0))
> +			goto out;
>  	} else if (format == SMK_CHANGE_FMT) {
>  		data[count] = '\0';
> -		if (smk_parse_long_rule(data, rule, 1, 1))
> -			goto out_free_rule;
> +		if (smk_parse_long_rule(data, &rule, 1, 1))
> +			goto out;
>  	} else {
>  		/*
>  		 * More on the minor hack for backward compatibility
>  		 */
>  		if (count == (SMK_OLOADLEN))
>  			data[SMK_OLOADLEN] = '-';
> -		if (smk_parse_rule(data, rule, 1))
> -			goto out_free_rule;
> +		if (smk_parse_rule(data, &rule, 1))
> +			goto out;
>  	}
>  
>  
>  	if (rule_list == NULL) {
>  		load = 1;
> -		skp = smk_find_entry(rule->smk_subject);
> +		skp = smk_find_entry(rule.smk_subject);
>  		rule_list = &skp->smk_rules;
>  		rule_lock = &skp->smk_rules_lock;
>  	}
>  
> -	rc = smk_set_access(rule, rule_list, rule_lock, load);
> -	if (rc == 0) {
> +	rc = smk_set_access(&rule, rule_list, rule_lock, load);
> +	if (rc == 0)
>  		rc = count;
> -		goto out;
> -	}
>  
> -out_free_rule:
> -	kfree(rule);
>  out:
>  	return rc;
>  }

Re: [RFC 4/5] security: smack: add kmem_cache for smack_rule allocations

[Casey Schaufler]

On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
> On ARM, sizeof(struct smack_rule)==20. Allocation by kmalloc() uses a
> 32-byte-long chunk to allocate 20 bytes. Just ask ksize().  It means that 40%
> of memory is simply wasted for padding bytes.
>
> The problem is fixed in this patch by using kmem_cache. The cache allocates
> struct smack_rule using 24-byte-long chunks according to ksize(). This reduces
> amount of used memory by 25%.

I'm not opposed to this change, but could I see some performance
numbers to justify it? In particular, I'm concerned about the rules
load impact.

> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> ---
>  security/smack/smack.h     |    3 +++
>  security/smack/smack_lsm.c |   11 ++++++++++-
>  security/smack/smackfs.c   |    2 +-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 8ad3095..38ba673 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -233,6 +233,9 @@ extern struct mutex	smack_known_lock;
>  extern struct list_head smack_known_list;
>  extern struct list_head smk_netlbladdr_list;
>  
> +/* Cache for fast and thrifty allocations */
> +extern struct kmem_cache *smack_rule_cache;
> +
>  extern struct security_operations smack_ops;
>  
>  /*
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index d52c780..7aa696a 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3564,6 +3564,9 @@ static __init void init_smack_known_list(void)
>  	list_add(&smack_known_web.list, &smack_known_list);
>  }
>  
> +/* KMEM caches for fast and thrifty allocations */
> +struct kmem_cache *smack_rule_cache;
> +
>  /**
>   * smack_init - initialize the smack system
>   *
> @@ -3577,10 +3580,16 @@ static __init int smack_init(void)
>  	if (!security_module_enable(&smack_ops))
>  		return 0;
>  
> +	smack_rule_cache = KMEM_CACHE(smack_rule, 0);
> +	if (!smack_rule_cache)
> +		return -ENOMEM;
> +
>  	tsp = new_task_smack(smack_known_floor.smk_known,
>  				smack_known_floor.smk_known, GFP_KERNEL);
> -	if (tsp == NULL)
> +	if (tsp == NULL) {
> +		kmem_cache_destroy(smack_rule_cache);
>  		return -ENOMEM;
> +	}
>  
>  	printk(KERN_INFO "Smack:  Initializing.\n");
>  
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index e8c57f3..c08b1ec 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -217,7 +217,7 @@ static int smk_set_access(struct smack_parsed_rule *srp,
>  	}
>  
>  	if (found == 0) {
> -		sp = kzalloc(sizeof(*sp), GFP_KERNEL);
> +		sp = kmem_cache_zalloc(smack_rule_cache, GFP_KERNEL);
>  		if (sp == NULL) {
>  			rc = -ENOMEM;
>  			goto out;

Re: [RFC 5/5] security: smack: add kmem_cache for smack_master_list allocations

[Casey Schaufler]

On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
> On ARM, sizeof(struct smack_master_list) == 12. Allocation by kmalloc() uses a
> 32-byte-long chunk to allocate 12 bytes. Just ask ksize().  It means that 63%
> of memory is simply wasted for padding bytes.
>
> The problem is fixed in this patch by using kmem_cache. The cache allocates
> struct smack_master_list using 16-byte-long chunks according to ksize(). This
> reduces amount of used memory by 50%.

As with patch 4, I need to see performance numbers. Saving 50%
is good, but if there are 20,000 rules you're only saving 320K
of memory.

>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> ---
>  security/smack/smack.h     |    7 +++++++
>  security/smack/smack_lsm.c |    8 ++++++++
>  security/smack/smackfs.c   |    8 ++------
>  3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 38ba673..463f818 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -194,6 +194,12 @@ struct smk_audit_info {
>  	struct smack_audit_data sad;
>  #endif
>  };
> +
> +struct smack_master_list {
> +	struct list_head	list;
> +	struct smack_rule	*smk_rule;
> +};
> +
>  /*
>   * These functions are in smack_lsm.c
>   */
> @@ -235,6 +241,7 @@ extern struct list_head smk_netlbladdr_list;
>  
>  /* Cache for fast and thrifty allocations */
>  extern struct kmem_cache *smack_rule_cache;
> +extern struct kmem_cache *smack_master_list_cache;
>  
>  extern struct security_operations smack_ops;
>  
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 7aa696a..1d4a1b0 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3566,6 +3566,7 @@ static __init void init_smack_known_list(void)
>  
>  /* KMEM caches for fast and thrifty allocations */
>  struct kmem_cache *smack_rule_cache;
> +struct kmem_cache *smack_master_list_cache;
>  
>  /**
>   * smack_init - initialize the smack system
> @@ -3584,9 +3585,16 @@ static __init int smack_init(void)
>  	if (!smack_rule_cache)
>  		return -ENOMEM;
>  
> +	smack_master_list_cache = KMEM_CACHE(smack_master_list, 0);
> +	if (!smack_master_list_cache) {
> +		kmem_cache_destroy(smack_rule_cache);
> +		return -ENOMEM;
> +	}
> +
>  	tsp = new_task_smack(smack_known_floor.smk_known,
>  				smack_known_floor.smk_known, GFP_KERNEL);
>  	if (tsp == NULL) {
> +		kmem_cache_destroy(smack_master_list_cache);
>  		kmem_cache_destroy(smack_rule_cache);
>  		return -ENOMEM;
>  	}
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index c08b1ec..c7a1b0d 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -104,11 +104,6 @@ LIST_HEAD(smk_netlbladdr_list);
>   * Rule lists are maintained for each label.
>   * This master list is just for reading /smack/load and /smack/load2.
>   */
> -struct smack_master_list {
> -	struct list_head	list;
> -	struct smack_rule	*smk_rule;
> -};
> -
>  LIST_HEAD(smack_rule_list);
>  
>  struct smack_parsed_rule {
> @@ -233,7 +228,8 @@ static int smk_set_access(struct smack_parsed_rule *srp,
>  		 * it needs to get added for reporting.
>  		 */
>  		if (global) {
> -			smlp = kzalloc(sizeof(*smlp), GFP_KERNEL);
> +			smlp = kmem_cache_zalloc(smack_master_list_cache,
> +							GFP_KERNEL);
>  			if (smlp != NULL) {
>  				smlp->smk_rule = sp;
>  				list_add_rcu(&smlp->list, &smack_rule_list);

Re: [RFC 1/5] security: smack: avoid kmalloc allocations while loading a rule string

[Tomasz Stanislawski]

Hi Casey,
Thank you for the review.
Please refer to the comments below.

On 06/15/2013 09:32 PM, Casey Schaufler wrote:
> On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
>> The maximal length for a rule line for long format is introduced as
>> SMK_LOAD2LEN. This allows a buffer for a rule string to be allocated
>> on a stack instead of a heap (aka kmalloc cache).
>>
>> Limiting the length of a rule line helps to avoid allocations of a very long
>> contiguous buffer from a heap if user calls write() for a very long chunk.
>> Such an allocation often causes a lot swapper/writeback havoc and it is very
>> likely to fails.
>>
>> Moreover, stack allocation is slightly faster than from kmalloc.
>>
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> 
> Please see the explanation below.
> 
> Nacked-by: Casey Schaufler <casey@schaufler-ca.com>
> 
>> ---
>>  security/smack/smackfs.c |   15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>> index 53a08b8..9a3cd0d 100644
>> --- a/security/smack/smackfs.c
>> +++ b/security/smack/smackfs.c
>> @@ -137,6 +137,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
>>   * SMK_ACCESS: Maximum possible combination of access permissions
>>   * SMK_ACCESSLEN: Maximum length for a rule access field
>>   * SMK_LOADLEN: Smack rule length
>> + * SMK_LOAD2LEN: Smack maximal long rule length excluding \0
>>   */
>>  #define SMK_OACCESS	"rwxa"
>>  #define SMK_ACCESS	"rwxat"
>> @@ -144,6 +145,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
>>  #define SMK_ACCESSLEN	(sizeof(SMK_ACCESS) - 1)
>>  #define SMK_OLOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_OACCESSLEN)
>>  #define SMK_LOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_ACCESSLEN)
>> +#define SMK_LOAD2LEN	(2 * SMK_LONGLABEL + SMK_ACCESSLEN + 2)
>>  
>>  /*
>>   * Stricly for CIPSO level manipulation.
>> @@ -447,8 +449,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>>  {
>>  	struct smack_known *skp;
>>  	struct smack_parsed_rule *rule;
>> -	char *data;
>> -	int datalen;
>> +	char data[SMK_LOAD2LEN + 1];
> 
> That puts over 512 bytes on the stack. The reason that the code
> uses a temporary allocation is that 512 bytes to considerably
> beyond what is considered reasonable to put on the kernel stack.
> As reasonable as this approach is in user space code, it is not
> appropriate in the kernel.
> 

OK. I see the problem now. Usually the kernel stack is limited to 8KiB (2 pages).
I agree that 512-byte allocation is not a good idea.
Anyway, I still think that a length of a rule should be limited.
This will protect from kmalloc() fro too long buffers.
What is your opinion?

>>  	int rc = -EINVAL;
>>  	int load = 0;
>>  
>> @@ -465,13 +466,10 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>>  		 */
>>  		if (count != SMK_OLOADLEN && count != SMK_LOADLEN)
>>  			return -EINVAL;
>> -		datalen = SMK_LOADLEN;
>> -	} else
>> -		datalen = count + 1;
>> +	}
>>  
>> -	data = kzalloc(datalen, GFP_KERNEL);
>> -	if (data == NULL)
>> -		return -ENOMEM;
>> +	if (count > SMK_LOAD2LEN)
>> +		count = SMK_LOAD2LEN;
>>  
>>  	if (copy_from_user(data, buf, count) != 0) {
>>  		rc = -EFAULT;
>> @@ -522,7 +520,6 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>>  out_free_rule:
>>  	kfree(rule);
>>  out:
>> -	kfree(data);
>>  	return rc;
>>  }
>>  
> 
> 

Re: [RFC 1/5] security: smack: avoid kmalloc allocations while loading a rule string

[Casey Schaufler]

On 6/17/2013 4:24 AM, Tomasz Stanislawski wrote:
> Hi Casey,
> Thank you for the review.
> Please refer to the comments below.
>
> On 06/15/2013 09:32 PM, Casey Schaufler wrote:
>> On 6/13/2013 8:29 AM, Tomasz Stanislawski wrote:
>>> The maximal length for a rule line for long format is introduced as
>>> SMK_LOAD2LEN. This allows a buffer for a rule string to be allocated
>>> on a stack instead of a heap (aka kmalloc cache).
>>>
>>> Limiting the length of a rule line helps to avoid allocations of a very long
>>> contiguous buffer from a heap if user calls write() for a very long chunk.
>>> Such an allocation often causes a lot swapper/writeback havoc and it is very
>>> likely to fails.
>>>
>>> Moreover, stack allocation is slightly faster than from kmalloc.
>>>
>>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> Please see the explanation below.
>>
>> Nacked-by: Casey Schaufler <casey@schaufler-ca.com>
>>
>>> ---
>>>  security/smack/smackfs.c |   15 ++++++---------
>>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
>>> index 53a08b8..9a3cd0d 100644
>>> --- a/security/smack/smackfs.c
>>> +++ b/security/smack/smackfs.c
>>> @@ -137,6 +137,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
>>>   * SMK_ACCESS: Maximum possible combination of access permissions
>>>   * SMK_ACCESSLEN: Maximum length for a rule access field
>>>   * SMK_LOADLEN: Smack rule length
>>> + * SMK_LOAD2LEN: Smack maximal long rule length excluding \0
>>>   */
>>>  #define SMK_OACCESS	"rwxa"
>>>  #define SMK_ACCESS	"rwxat"
>>> @@ -144,6 +145,7 @@ const char *smack_cipso_option = SMACK_CIPSO_OPTION;
>>>  #define SMK_ACCESSLEN	(sizeof(SMK_ACCESS) - 1)
>>>  #define SMK_OLOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_OACCESSLEN)
>>>  #define SMK_LOADLEN	(SMK_LABELLEN + SMK_LABELLEN + SMK_ACCESSLEN)
>>> +#define SMK_LOAD2LEN	(2 * SMK_LONGLABEL + SMK_ACCESSLEN + 2)
>>>  
>>>  /*
>>>   * Stricly for CIPSO level manipulation.
>>> @@ -447,8 +449,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>>>  {
>>>  	struct smack_known *skp;
>>>  	struct smack_parsed_rule *rule;
>>> -	char *data;
>>> -	int datalen;
>>> +	char data[SMK_LOAD2LEN + 1];
>> That puts over 512 bytes on the stack. The reason that the code
>> uses a temporary allocation is that 512 bytes to considerably
>> beyond what is considered reasonable to put on the kernel stack.
>> As reasonable as this approach is in user space code, it is not
>> appropriate in the kernel.
>>
> OK. I see the problem now. Usually the kernel stack is limited to 8KiB (2 pages).
> I agree that 512-byte allocation is not a good idea.
> Anyway, I still think that a length of a rule should be limited.
> This will protect from kmalloc() fro too long buffers.
> What is your opinion?

I agree that range checking is good. I am toying with how to
allow multiple rules per write. If we go that route the number
will likely be more like 4k than 512.

>
>>>  	int rc = -EINVAL;
>>>  	int load = 0;
>>>  
>>> @@ -465,13 +466,10 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>>>  		 */
>>>  		if (count != SMK_OLOADLEN && count != SMK_LOADLEN)
>>>  			return -EINVAL;
>>> -		datalen = SMK_LOADLEN;
>>> -	} else
>>> -		datalen = count + 1;
>>> +	}
>>>  
>>> -	data = kzalloc(datalen, GFP_KERNEL);
>>> -	if (data == NULL)
>>> -		return -ENOMEM;
>>> +	if (count > SMK_LOAD2LEN)
>>> +		count = SMK_LOAD2LEN;
>>>  
>>>  	if (copy_from_user(data, buf, count) != 0) {
>>>  		rc = -EFAULT;
>>> @@ -522,7 +520,6 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>>>  out_free_rule:
>>>  	kfree(rule);
>>>  out:
>>> -	kfree(data);
>>>  	return rc;
>>>  }
>>>  
>>
>

[PATCH] security: smack: fix memleak in smk_write_rules_list()

[Tomasz Stanislawski]

>From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001
From: Tomasz Stanislawski <t.stanislaws@samsung.com>
Date: Thu, 6 Jun 2013 09:30:50 +0200
Subject: [PATCH] security: smack: fix memleak in smk_write_rules_list()

The smack_parsed_rule structure is allocated.  If a rule is successfully
installed then the last reference to the object is lost.  This patch fixes this
leak. Moreover smack_parsed_rule is allocated on stack because it no longer
needed ofter smk_write_rules_list() is finished.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 53a08b8..08aebc2 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -446,7 +446,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 					struct mutex *rule_lock, int format)
 {
 	struct smack_known *skp;
-	struct smack_parsed_rule *rule;
+	struct smack_parsed_rule rule;
 	char *data;
 	int datalen;
 	int rc = -EINVAL;
@@ -478,49 +478,39 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
 		goto out;
 	}

-	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
-	if (rule == NULL) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
 	if (format == SMK_LONG_FMT) {
 		/*
 		 * Be sure the data string is terminated.
 		 */
 		data[count] = '\0';
-		if (smk_parse_long_rule(data, rule, 1, 0))
-			goto out_free_rule;
+		if (smk_parse_long_rule(data, &rule, 1, 0))
+			goto out;
 	} else if (format == SMK_CHANGE_FMT) {
 		data[count] = '\0';
-		if (smk_parse_long_rule(data, rule, 1, 1))
-			goto out_free_rule;
+		if (smk_parse_long_rule(data, &rule, 1, 1))
+			goto out;
 	} else {
 		/*
 		 * More on the minor hack for backward compatibility
 		 */
 		if (count == (SMK_OLOADLEN))
 			data[SMK_OLOADLEN] = '-';
-		if (smk_parse_rule(data, rule, 1))
-			goto out_free_rule;
+		if (smk_parse_rule(data, &rule, 1))
+			goto out;
 	}


 	if (rule_list == NULL) {
 		load = 1;
-		skp = smk_find_entry(rule->smk_subject);
+		skp = smk_find_entry(rule.smk_subject);
 		rule_list = &skp->smk_rules;
 		rule_lock = &skp->smk_rules_lock;
 	}

-	rc = smk_set_access(rule, rule_list, rule_lock, load);
-	if (rc == 0) {
+	rc = smk_set_access(&rule, rule_list, rule_lock, load);
+	if (rc == 0)
 		rc = count;
-		goto out;
-	}

-out_free_rule:
-	kfree(rule);
 out:
 	kfree(data);
 	return rc;
-- 
1.7.9.5

Re: [PATCH] security: smack: fix memleak in smk_write_rules_list()

[Casey Schaufler]

On 6/19/2013 7:08 AM, Tomasz Stanislawski wrote:
> >From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001
> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Date: Thu, 6 Jun 2013 09:30:50 +0200
> Subject: [PATCH] security: smack: fix memleak in smk_write_rules_list()
>
> The smack_parsed_rule structure is allocated.  If a rule is successfully
> installed then the last reference to the object is lost.  This patch fixes this
> leak. Moreover smack_parsed_rule is allocated on stack because it no longer
> needed ofter smk_write_rules_list() is finished.
>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>

I will add this patch to the smack-next tree.

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/smack/smackfs.c |   30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 53a08b8..08aebc2 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -446,7 +446,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  					struct mutex *rule_lock, int format)
>  {
>  	struct smack_known *skp;
> -	struct smack_parsed_rule *rule;
> +	struct smack_parsed_rule rule;
>  	char *data;
>  	int datalen;
>  	int rc = -EINVAL;
> @@ -478,49 +478,39 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  		goto out;
>  	}
>
> -	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> -	if (rule == NULL) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> -
>  	if (format == SMK_LONG_FMT) {
>  		/*
>  		 * Be sure the data string is terminated.
>  		 */
>  		data[count] = '\0';
> -		if (smk_parse_long_rule(data, rule, 1, 0))
> -			goto out_free_rule;
> +		if (smk_parse_long_rule(data, &rule, 1, 0))
> +			goto out;
>  	} else if (format == SMK_CHANGE_FMT) {
>  		data[count] = '\0';
> -		if (smk_parse_long_rule(data, rule, 1, 1))
> -			goto out_free_rule;
> +		if (smk_parse_long_rule(data, &rule, 1, 1))
> +			goto out;
>  	} else {
>  		/*
>  		 * More on the minor hack for backward compatibility
>  		 */
>  		if (count == (SMK_OLOADLEN))
>  			data[SMK_OLOADLEN] = '-';
> -		if (smk_parse_rule(data, rule, 1))
> -			goto out_free_rule;
> +		if (smk_parse_rule(data, &rule, 1))
> +			goto out;
>  	}
>
>
>  	if (rule_list == NULL) {
>  		load = 1;
> -		skp = smk_find_entry(rule->smk_subject);
> +		skp = smk_find_entry(rule.smk_subject);
>  		rule_list = &skp->smk_rules;
>  		rule_lock = &skp->smk_rules_lock;
>  	}
>
> -	rc = smk_set_access(rule, rule_list, rule_lock, load);
> -	if (rc == 0) {
> +	rc = smk_set_access(&rule, rule_list, rule_lock, load);
> +	if (rc == 0)
>  		rc = count;
> -		goto out;
> -	}
>
> -out_free_rule:
> -	kfree(rule);
>  out:
>  	kfree(data);
>  	return rc;

Re: [PATCH] security: smack: fix memleak in smk_write_rules_list()

[Casey Schaufler]

On 6/19/2013 7:08 AM, Tomasz Stanislawski wrote:
> >From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001
> From: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Date: Thu, 6 Jun 2013 09:30:50 +0200
> Subject: [PATCH] security: smack: fix memleak in smk_write_rules_list()
>
> The smack_parsed_rule structure is allocated.  If a rule is successfully
> installed then the last reference to the object is lost.  This patch fixes this
> leak. Moreover smack_parsed_rule is allocated on stack because it no longer
> needed ofter smk_write_rules_list() is finished.
>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>


Applied to git://git.gitorious.org/smack-next/kernel.git#smack-for-3.12

Rebasing was required. The change has been tested.

> ---
>  security/smack/smackfs.c |   30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 53a08b8..08aebc2 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -446,7 +446,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  					struct mutex *rule_lock, int format)
>  {
>  	struct smack_known *skp;
> -	struct smack_parsed_rule *rule;
> +	struct smack_parsed_rule rule;
>  	char *data;
>  	int datalen;
>  	int rc = -EINVAL;
> @@ -478,49 +478,39 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
>  		goto out;
>  	}
>
> -	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> -	if (rule == NULL) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> -
>  	if (format == SMK_LONG_FMT) {
>  		/*
>  		 * Be sure the data string is terminated.
>  		 */
>  		data[count] = '\0';
> -		if (smk_parse_long_rule(data, rule, 1, 0))
> -			goto out_free_rule;
> +		if (smk_parse_long_rule(data, &rule, 1, 0))
> +			goto out;
>  	} else if (format == SMK_CHANGE_FMT) {
>  		data[count] = '\0';
> -		if (smk_parse_long_rule(data, rule, 1, 1))
> -			goto out_free_rule;
> +		if (smk_parse_long_rule(data, &rule, 1, 1))
> +			goto out;
>  	} else {
>  		/*
>  		 * More on the minor hack for backward compatibility
>  		 */
>  		if (count == (SMK_OLOADLEN))
>  			data[SMK_OLOADLEN] = '-';
> -		if (smk_parse_rule(data, rule, 1))
> -			goto out_free_rule;
> +		if (smk_parse_rule(data, &rule, 1))
> +			goto out;
>  	}
>
>
>  	if (rule_list == NULL) {
>  		load = 1;
> -		skp = smk_find_entry(rule->smk_subject);
> +		skp = smk_find_entry(rule.smk_subject);
>  		rule_list = &skp->smk_rules;
>  		rule_lock = &skp->smk_rules_lock;
>  	}
>
> -	rc = smk_set_access(rule, rule_list, rule_lock, load);
> -	if (rc == 0) {
> +	rc = smk_set_access(&rule, rule_list, rule_lock, load);
> +	if (rc == 0)
>  		rc = count;
> -		goto out;
> -	}
>
> -out_free_rule:
> -	kfree(rule);
>  out:
>  	kfree(data);
>  	return rc;