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 <[email protected]>
>
> Please see the explanation below.
>
> Nacked-by: Casey Schaufler <[email protected]>
>
>> ---
>> 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;
>> }
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
