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

From: Casey Schaufler
Date: Mon Jun 17 2013 - 18:45:20 EST


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@xxxxxxxxxxx>
>> Please see the explanation below.
>>
>> Nacked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>
>>> ---
>>> 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;
>>> }
>>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/