Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1740490

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

Show all headers | View raw

* 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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] params: Fix an overflow in param_attr_show Jean Delvare <jdelvare@suse.de> - 2017-09-27 10:20 +0200
  Re: [PATCH] params: Fix an overflow in param_attr_show Ingo Molnar <mingo@kernel.org> - 2017-09-27 10:30 +0200
    Re: [PATCH] params: Fix an overflow in param_attr_show Jean Delvare <jdelvare@suse.de> - 2017-09-27 11:50 +0200
      Re: [PATCH] params: Fix an overflow in param_attr_show Ingo Molnar <mingo@kernel.org> - 2017-09-27 15:40 +0200
        Re: [PATCH] params: Fix an overflow in param_attr_show Jean Delvare <jdelvare@suse.de> - 2017-09-28 10:10 +0200
          Re: [PATCH] params: Fix an overflow in param_attr_show Jean Delvare <jdelvare@suse.de> - 2017-09-28 10:20 +0200
            Re: [PATCH] params: Fix an overflow in param_attr_show Ingo Molnar <mingo@kernel.org> - 2017-09-28 10:40 +0200
              Re: [PATCH] params: Fix an overflow in param_attr_show Jean Delvare <jdelvare@suse.de> - 2017-09-28 18:10 +0200
          Re: [PATCH] params: Fix an overflow in param_attr_show Ingo Molnar <mingo@kernel.org> - 2017-09-28 10:50 +0200
  Re: [PATCH] params: Fix an overflow in param_attr_show Jean Delvare <jdelvare@suse.de> - 2017-09-28 11:00 +0200

csiph-web