Re: [PATCH] params: Fix an overflow in param_attr_show

[Ingo Molnar <mingo@kernel.org>]

* Jean Delvare <jdelvare@suse.de> wrote:

> Function param_attr_show could overflow the buffer it is operating
> on. The buffer size is PAGE_SIZE, and the string returned by
> attribute->param->ops->get is generated by scnprintf(buffer,
> PAGE_SIZE, ...) so it could be PAGE_SIZE - 1 long, with the
> terminating '\0' at the very end of the buffer. Calling
> strcat(..., "\n") on this isn't safe, as the '\0' will be replaced
> by '\n' (OK) and then another '\0' will be added past the end of
> the buffer (not OK.)
> 
> Simply add the trailing '\n' when writing the attribute contents to
> the buffer originally. This is safe, and also faster.
> 
> Credits to Teradata for discovering this issue.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
>  kernel/params.c |   22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> --- linux-4.13.orig/kernel/params.c	2017-09-19 16:07:18.794254776 +0200
> +++ linux-4.13/kernel/params.c	2017-09-19 16:12:57.398426205 +0200
> @@ -236,14 +236,14 @@ char *parse_args(const char *doing,
>  	EXPORT_SYMBOL(param_ops_##name)
>  
>  
> -STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);
> -STANDARD_PARAM_DEF(short, short, "%hi", kstrtos16);
> -STANDARD_PARAM_DEF(ushort, unsigned short, "%hu", kstrtou16);
> -STANDARD_PARAM_DEF(int, int, "%i", kstrtoint);
> -STANDARD_PARAM_DEF(uint, unsigned int, "%u", kstrtouint);
> -STANDARD_PARAM_DEF(long, long, "%li", kstrtol);
> -STANDARD_PARAM_DEF(ulong, unsigned long, "%lu", kstrtoul);
> -STANDARD_PARAM_DEF(ullong, unsigned long long, "%llu", kstrtoull);
> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8);
> +STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16);
> +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16);
> +STANDARD_PARAM_DEF(int, int, "%i\n", kstrtoint);
> +STANDARD_PARAM_DEF(uint, unsigned int, "%u\n", kstrtouint);
> +STANDARD_PARAM_DEF(long, long, "%li\n", kstrtol);
> +STANDARD_PARAM_DEF(ulong, unsigned long, "%lu\n", kstrtoul);
> +STANDARD_PARAM_DEF(ullong, unsigned long long, "%llu\n", kstrtoull);
>  
>  int param_set_charp(const char *val, const struct kernel_param *kp)
>  {
> @@ -270,7 +270,7 @@ EXPORT_SYMBOL(param_set_charp);
>  
>  int param_get_charp(char *buffer, const struct kernel_param *kp)
>  {
> -	return scnprintf(buffer, PAGE_SIZE, "%s", *((char **)kp->arg));
> +	return scnprintf(buffer, PAGE_SIZE, "%s\n", *((char **)kp->arg));
>  }
>  EXPORT_SYMBOL(param_get_charp);
>  
> @@ -549,10 +549,6 @@ static ssize_t param_attr_show(struct mo
>  	kernel_param_lock(mk->mod);
>  	count = attribute->param->ops->get(buf, attribute->param);
>  	kernel_param_unlock(mk->mod);
> -	if (count > 0) {
> -		strcat(buf, "\n");
> -		++count;
> -	}
>  	return count;
>  }

So the \n additions to the STANDARD_PARAM_DEF() lines

> +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8);
> +STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16);
> +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16);

are not necessary anymore, with the other changes? If so then I'd leave them 
without the \n - that's also easier to read.

Or if adding this:

	STANDARD_PARAM_DEF(byte, unsigned char, "%hhu", kstrtou8);

... is still unsafe then I'd suggest making it safe - it's easy to miss the lack 
of a \n during review and testing.

Thanks,

	Ingo