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

[Jean Delvare]

Hi Ingo,

On mer., 2017-09-27 at 10:26 +0200, Ingo Molnar wrote:
> * Jean Delvare <jdelvare@xxxxxxx> 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@xxxxxxx>
> > ---
> >  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.

What other changes are you referring to? I'm confused. Are you sure you
read the patch entirely before commenting on it?

> 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.

Why would you add this when it's already present? Confused again.

To answer the question, even if I don't get the point, omitting the
trailing '\n' would be safe in the sense that it would not cause a
buffer overflow. It would be wrong in the sense that reading from the
sysfs attribute would miss the trailing '\n'. But basic testing would
catch that easily, contrary to your claim above. If review did not
catch it before, that is, and it should, it ain't that hard really.

I'm curious, have you decided to bash every patch I post just to make
my life harder? It's working, congratulations.

-- 
Jean Delvare
SUSE L3 Support