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

[Ingo Molnar <mingo@kernel.org>]

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

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

I was referring to the rest of the patch, which avoids the overflow even if the \n 
is not present in the pattern.

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

So what I was asking, what happens if someone adds a new entry and
forgets the \n?

This is not hypothetical - for example this commit:

  b4210b810e50 ("Add module param type 'ullong'")

... added a new entry for a new param type. It's entirely possible for
new additions to happen here.

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

Yeah, I was mainly asking whether any overflow could happen even if
the \n is missing erroneously- because it was not clear to me from
your patch. It's good that it cannot.

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

I review almost every patch I get sent and the unsafe/unrobust string
pattern caught my attention. I did not bash your patch, and I have no
idea why answering review questions should be making your life harder.

At minimum I'd suggest aligning the definitions vertically, to make sure
any missing \n stands out more, visually:

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

Thanks,

	Ingo