Optimizations for memory handling in smk_write_rules_list()

RFC NAK Tomasz Stanislawski 2 participants 16 messages Jun 13, 2013 – Aug 1, 2013

Subsystems (2)

Security (LSM) Smack security module

lore

Share & tools

lore

Tomasz Stanislawski

author

2013-06-13 15:29 UTC #0 lore raw

cover Optimizations for memory handling in smk_write_rules_list()

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(-)

Signature · 1 line

1.7.9.5

Tomasz Stanislawski

author

2013-06-13 15:29 UTC #1 lore raw

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

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 <[email protected]>

---
 security/smack/smackfs.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

security/smack/smackfs.c +6 -9

index 53a08b8..9a3cd0d 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Casey Schaufler

maintainer Nacked-by on v1 1/5 NAK

2013-06-15 19:32 UTC #2 lore raw

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

Quoted 4 lines · 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).

Quoted 8 lines

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 <[email protected]>

Please see the explanation below.

Nacked-by Casey Schaufler <[email protected]>

Quoted 25 lines

---
 security/smack/smackfs.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

security/smack/smackfs.c +2 -0

index 53a08b8..9a3cd0d 100644
@@ -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.

security/smack/smackfs.c static ssize_t smk_write_rules_list(struct file *file, const char __user *buf, · line 452

Quoted 6 lines

security/smack/smackfs.c +1 -2

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

Quoted 28 lines

security/smack/smackfs.c +3 -7

 	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;
 }

Tomasz Stanislawski

author

2013-06-17 11:24 UTC #3 lore raw

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

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

Quoted 40 lines · Casey Schaufler

Quoted 4 lines · 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).
Quoted 8 lines
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 <[email protected]>
Please see the explanation below.
Nacked-by Casey Schaufler <[email protected]>
Quoted 22 lines
---
 security/smack/smackfs.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
security/smack/smackfs.c +2 -0
index 53a08b8..9a3cd0d 100644
@@ -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)
 

Quoted 16 lines

security/smack/smackfs.c static ssize_t smk_write_rules_list(struct file *file, const char __user *buf, · line 452
Quoted 9 lines
security/smack/smackfs.c +1 -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?

Quoted 30 lines

Quoted 28 lines

security/smack/smackfs.c +3 -7

 	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;
 }

Casey Schaufler

maintainer

2013-06-17 22:45 UTC #4 lore raw

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

Quoted 59 lines · Tomasz Stanislawski

Hi Casey,
Thank you for the review.
Please refer to the comments below.
Quoted 39 lines · Casey Schaufler
Quoted 4 lines · 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).
Quoted 8 lines
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 <[email protected]>
Please see the explanation below.
Nacked-by Casey Schaufler <[email protected]>
Quoted 22 lines
---
 security/smack/smackfs.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
security/smack/smackfs.c +2 -0
index 53a08b8..9a3cd0d 100644
@@ -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)
 
Quoted 15 lines
security/smack/smackfs.c static ssize_t smk_write_rules_list(struct file *file, const char __user *buf, · line 452
Quoted 9 lines
security/smack/smackfs.c +1 -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.

Quoted 5 lines

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.

Quoted 31 lines

Quoted 30 lines

Quoted 29 lines

security/smack/smackfs.c +3 -7

 	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;
 }

Tomasz Stanislawski

author

2013-06-13 15:29 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 security/smack/smackfs.c |   67 +++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

security/smack/smackfs.c +33 ~~-34~~

index 9a3cd0d..46f111e 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Casey Schaufler

maintainer

2013-06-15 19:41 UTC #6 lore raw

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

Quoted 5 lines · 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.

Quoted 8 lines

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 <[email protected]>

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

Quoted 43 lines

---
 security/smack/smackfs.c |   67 +++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

security/smack/smackfs.c +27 -0

index 9a3cd0d..46f111e 100644
@@ -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

security/smack/smackfs.c static int smk_parse_rule(const char *data, struct smack_parsed_rule *rule, · line 405

Quoted 9 lines

security/smack/smackfs.c +1 -5

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

Quoted 45 lines

security/smack/smackfs.c +5 ~~-29~~

+	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;
 }

Tomasz Stanislawski

author

2013-06-13 15:29 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

security/smack/smackfs.c +10 ~~-20~~

index 46f111e..e8c57f3 100644
@@ -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;
 }

Signature · 1 line

1.7.9.5

Casey Schaufler

maintainer

2013-06-15 19:54 UTC #8 lore raw

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

Quoted 6 lines · 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 <[email protected]>

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.

Quoted 76 lines

---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

security/smack/smackfs.c +10 ~~-20~~

index 46f111e..e8c57f3 100644
@@ -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;
 }

Tomasz Stanislawski

author

2013-06-13 15:29 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 security/smack/smack.h     |    3 +++
 security/smack/smack_lsm.c |   11 ++++++++++-
 security/smack/smackfs.c   |    2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

security/smack/smack.h +3 -0

index 8ad3095..38ba673 100644
@@ -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;
 
 /*

security/smack/smack_lsm.c +10 -1

index d52c780..7aa696a 100644
@@ -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");

security/smack/smackfs.c +1 -1

index e8c57f3..c08b1ec 100644
@@ -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;

Signature · 1 line

1.7.9.5

Casey Schaufler

maintainer

2013-06-15 20:00 UTC #10 lore raw

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

Quoted 7 lines · 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%.

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.

Quoted 66 lines

Signed-off-by Tomasz Stanislawski <[email protected]>

---
 security/smack/smack.h     |    3 +++
 security/smack/smack_lsm.c |   11 ++++++++++-
 security/smack/smackfs.c   |    2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

security/smack/smack.h +3 -0

index 8ad3095..38ba673 100644
@@ -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;
 
 /*

security/smack/smack_lsm.c +10 -1

index d52c780..7aa696a 100644
@@ -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");

security/smack/smackfs.c +1 -1

index e8c57f3..c08b1ec 100644
@@ -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;

Tomasz Stanislawski

author

2013-06-13 15:29 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 security/smack/smack.h     |    7 +++++++
 security/smack/smack_lsm.c |    8 ++++++++
 security/smack/smackfs.c   |    8 ++------
 3 files changed, 17 insertions(+), 6 deletions(-)

security/smack/smack.h +7 -0

index 38ba673..463f818 100644
@@ -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;

security/smack/smack_lsm.c +8 -0

index 7aa696a..1d4a1b0 100644
@@ -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;
 	}

security/smack/smackfs.c +2 -6

index c08b1ec..c7a1b0d 100644
@@ -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);

Signature · 1 line

1.7.9.5

Casey Schaufler

maintainer

2013-06-15 20:08 UTC #12 lore raw

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

Quoted 7 lines · 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%.

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.

Quoted 88 lines

Signed-off-by Tomasz Stanislawski <[email protected]>

---
 security/smack/smack.h     |    7 +++++++
 security/smack/smack_lsm.c |    8 ++++++++
 security/smack/smackfs.c   |    8 ++------
 3 files changed, 17 insertions(+), 6 deletions(-)

security/smack/smack.h +7 -0

index 38ba673..463f818 100644
@@ -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;

security/smack/smack_lsm.c +8 -0

index 7aa696a..1d4a1b0 100644
@@ -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;
 	}

security/smack/smackfs.c +2 -6

index c08b1ec..c7a1b0d 100644
@@ -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);

Tomasz Stanislawski

author

2013-06-19 14:08 UTC #13 lore raw

PATCH 1/1 security: smack: fix memleak in smk_write_rules_list()

In reply to Tomasz Stanislawski

Quoted 1 line

From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001

From: Tomasz Stanislawski <[email protected]>
Date: Thu, 6 Jun 2013 09:30:50 +0200
Subject: [PATCH] security: smack: fix memleak in smk_write_rules_list()

Signed-off-by Tomasz Stanislawski <[email protected]>

---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

security/smack/smackfs.c +10 ~~-20~~

index 53a08b8..08aebc2 100644
@@ -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;

Signature · 1 line

1.7.9.5

Casey Schaufler

maintainer Acked-by on v1 1/1

2013-06-28 19:40 UTC #14 lore raw

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

Quoted 11 lines · Tomasz Stanislawski

Quoted 1 line
From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001
From: Tomasz Stanislawski <[email protected]>
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 <[email protected]>

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

Acked-by Casey Schaufler <[email protected]>

Quoted 76 lines

---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

security/smack/smackfs.c +10 ~~-20~~

index 53a08b8..08aebc2 100644
@@ -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;

Casey Schaufler

maintainer Acked-by on v1 1/1 Applied

2013-08-01 20:01 UTC #15 lore raw

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

In reply to Tomasz Stanislawski

Quoted 11 lines · Tomasz Stanislawski

Quoted 1 line
From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001
From: Tomasz Stanislawski <[email protected]>
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 <[email protected]>

Acked-by Casey Schaufler <[email protected]>

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

Rebasing was required. The change has been tested.

Quoted 76 lines

---
 security/smack/smackfs.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

security/smack/smackfs.c +10 ~~-20~~

index 53a08b8..08aebc2 100644
@@ -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;