Messages in this thread | | | Date | Mon, 17 Jun 2013 15:38:27 -0700 | From | Casey Schaufler <> | Subject | Re: [RFC 1/5] security: smack: avoid kmalloc allocations while loading a rule string |
| |
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; >>> } >>> >> >
| |