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

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

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.

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