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

Groups > linux.kernel > #1740482 > unrolled thread

[PATCH] params: Fix an overflow in param_attr_show

Back to article view | Back to linux.kernel

Contents

  [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

#1740482 — [PATCH] params: Fix an overflow in param_attr_show

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


-- 
Jean Delvare
SUSE L3 Support

[toc] | [next] | [standalone]

#1740490

* 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

[toc] | [prev] | [next] | [standalone]

#1740575

Hi Ingo,

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

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

[toc] | [prev] | [next] | [standalone]

#1740731

* 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

[toc] | [prev] | [next] | [standalone]

#1741259

On Wed, 27 Sep 2017 15:31:04 +0200, Ingo Molnar wrote:
> * Jean Delvare <jdelvare@suse.de> wrote:
> > > 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.

You make things sound complex, when my patch is so simple. I'm simply
changing the point at which the trailing \n is added. The \n must be
present in the pattern, so "even if the \n is not present in the
pattern" was out of scope. And it turns out that it doesn't matter at
all anyway.

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

Sure it is possible to add a new parameter type. But why would the
person adding it forget the \n? I can't imagine that someone adding a
new type would type the new line of code character by character. Such an
operation is calling for copy, paste and edit, at which point there is
no reason why the \n would be actively deleted. Or this is sabotage,
really ;-)

Aligning parameters vertically as you suggest above is probably a good
idea for overall readability anyway, so I can change my patch to do
that, as I am modifying these lines anyway. It is pretty much
independent from the fix per se, but if it makes you happy...

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1741267

On Thu, 28 Sep 2017 10:02:23 +0200, Jean Delvare wrote:
> On Wed, 27 Sep 2017 15:31:04 +0200, Ingo Molnar wrote:
> > 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);  
> 
> Sure it is possible to add a new parameter type. But why would the
> person adding it forget the \n? I can't imagine that someone adding a
> new type would type the new line of code character by character. Such an
> operation is calling for copy, paste and edit, at which point there is
> no reason why the \n would be actively deleted. Or this is sabotage,
> really ;-)
> 
> Aligning parameters vertically as you suggest above is probably a good
> idea for overall readability anyway, so I can change my patch to do
> that, as I am modifying these lines anyway. It is pretty much
> independent from the fix per se, but if it makes you happy...

Or... I could append the \n inside the STANDARD_PARAM_DEF macro, so the
calls are unchanged. Makes my patch smaller, and addresses your concern
just as well, I suppose.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1741295

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

> On Thu, 28 Sep 2017 10:02:23 +0200, Jean Delvare wrote:
> > On Wed, 27 Sep 2017 15:31:04 +0200, Ingo Molnar wrote:
> > > 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);  
> > 
> > Sure it is possible to add a new parameter type. But why would the
> > person adding it forget the \n? I can't imagine that someone adding a
> > new type would type the new line of code character by character. Such an
> > operation is calling for copy, paste and edit, at which point there is
> > no reason why the \n would be actively deleted. Or this is sabotage,
> > really ;-)
> > 
> > Aligning parameters vertically as you suggest above is probably a good
> > idea for overall readability anyway, so I can change my patch to do
> > that, as I am modifying these lines anyway. It is pretty much
> > independent from the fix per se, but if it makes you happy...
> 
> Or... I could append the \n inside the STANDARD_PARAM_DEF macro, so the
> calls are unchanged. Makes my patch smaller, and addresses your concern
> just as well, I suppose.

Yeah, that would be even better:

  Acked-by: Ingo Molnar <mingo@kernel.org>

Note that the vertical alignment makes things easier to read regardless of the \n.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1741679

On Thu, 28 Sep 2017 10:38:27 +0200, Ingo Molnar wrote:
> * Jean Delvare <jdelvare@suse.de> wrote:
> > Or... I could append the \n inside the STANDARD_PARAM_DEF macro, so the
> > calls are unchanged. Makes my patch smaller, and addresses your concern
> > just as well, I suppose.  
> 
> Yeah, that would be even better:
> 
>   Acked-by: Ingo Molnar <mingo@kernel.org>
> 
> Note that the vertical alignment makes things easier to read regardless of the \n.

Agreed, but now I'll make it a separate patch.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1741299

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

> > 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);
> 
> Sure it is possible to add a new parameter type. But why would the
> person adding it forget the \n?

Because they are human? I certainly forgot similar details when writing code, 
numerous times, and making constructs more robust against mistakes is half of
my job as a maintainer. This is kernel design 101.

> I can't imagine that someone adding a
> new type would type the new line of code character by character. Such an
> operation is calling for copy, paste and edit, at which point there is
> no reason why the \n would be actively deleted. Or this is sabotage,
> really ;-)

WTF? Really, I've given you useful feedback in the last couple of days, and my 
suggestions were generally correct and on topic, still your replies were 
passive-aggressive, obtuse and generally foul tempered in every single case.

Just the latest example:

> Aligning parameters vertically as you suggest above is probably a good
> idea for overall readability anyway, so I can change my patch to do
> that, as I am modifying these lines anyway. It is pretty much
> independent from the fix per se, but if it makes you happy...

I made a routine, technically valid suggestion that I made countless other
kernel developers in the past who sent me code with such a pattern, and
I do not appreciate your condescending tone, it's not about 'making me happy'.

You need to handle criticism of your patches properly and constructively.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1741305

On Wed, 27 Sep 2017 10:10:31 +0200, Jean Delvare 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(-)
> (...)

This patch turns out to be broken, so please disregard for now. I
missed several types, like bool, invbool and string (not sure why we
have types charp and string which seems to serve the same purpose...)
and most importantly arrays of parameters, which very much assume that
individual parameters are not \n-terminated. There had to be a reason
for the current code...

Sorry for the noise, I'll work some more on it and see how to address
all the remaining issues.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web