Messages in this thread | | | Date | Mon, 17 Jun 2013 13:24:05 +0200 | From | Tomasz Stanislawski <> | Subject | 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.
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; >> } >> > >
| |