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

[Casey Schaufler <casey@schaufler-ca.com>]

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