* [PATCH] params: Fix an overflow in param_attr_show @ 2017-09-27 8:10 Jean Delvare 2017-09-27 8:26 ` Ingo Molnar 2017-09-28 8:49 ` Jean Delvare 0 siblings, 2 replies; 10+ messages in thread From: Jean Delvare @ 2017-09-27 8:10 UTC (permalink / raw) To: LKML; +Cc: Andrew Morton, Ingo Molnar, Baoquan He 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-27 8:10 [PATCH] params: Fix an overflow in param_attr_show Jean Delvare @ 2017-09-27 8:26 ` Ingo Molnar 2017-09-27 9:40 ` Jean Delvare 2017-09-28 8:49 ` Jean Delvare 1 sibling, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2017-09-27 8:26 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-27 8:26 ` Ingo Molnar @ 2017-09-27 9:40 ` Jean Delvare 2017-09-27 13:31 ` Ingo Molnar 0 siblings, 1 reply; 10+ messages in thread From: Jean Delvare @ 2017-09-27 9:40 UTC (permalink / raw) To: Ingo Molnar Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-27 9:40 ` Jean Delvare @ 2017-09-27 13:31 ` Ingo Molnar 2017-09-28 8:02 ` Jean Delvare 0 siblings, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2017-09-27 13:31 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-27 13:31 ` Ingo Molnar @ 2017-09-28 8:02 ` Jean Delvare 2017-09-28 8:11 ` Jean Delvare 2017-09-28 8:48 ` Ingo Molnar 0 siblings, 2 replies; 10+ messages in thread From: Jean Delvare @ 2017-09-28 8:02 UTC (permalink / raw) To: Ingo Molnar Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-28 8:02 ` Jean Delvare @ 2017-09-28 8:11 ` Jean Delvare 2017-09-28 8:38 ` Ingo Molnar 2017-09-28 8:48 ` Ingo Molnar 1 sibling, 1 reply; 10+ messages in thread From: Jean Delvare @ 2017-09-28 8:11 UTC (permalink / raw) To: Ingo Molnar Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-28 8:11 ` Jean Delvare @ 2017-09-28 8:38 ` Ingo Molnar 2017-09-28 13:33 ` Jean Delvare 0 siblings, 1 reply; 10+ messages in thread From: Ingo Molnar @ 2017-09-28 8:38 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-28 8:38 ` Ingo Molnar @ 2017-09-28 13:33 ` Jean Delvare 0 siblings, 0 replies; 10+ messages in thread From: Jean Delvare @ 2017-09-28 13:33 UTC (permalink / raw) To: Ingo Molnar Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-28 8:02 ` Jean Delvare 2017-09-28 8:11 ` Jean Delvare @ 2017-09-28 8:48 ` Ingo Molnar 1 sibling, 0 replies; 10+ messages in thread From: Ingo Molnar @ 2017-09-28 8:48 UTC (permalink / raw) To: Jean Delvare Cc: LKML, Andrew Morton, Baoquan He, Linus Torvalds, Thomas Gleixner, Peter Zijlstra, H. Peter Anvin, Borislav Petkov * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] params: Fix an overflow in param_attr_show 2017-09-27 8:10 [PATCH] params: Fix an overflow in param_attr_show Jean Delvare 2017-09-27 8:26 ` Ingo Molnar @ 2017-09-28 8:49 ` Jean Delvare 1 sibling, 0 replies; 10+ messages in thread From: Jean Delvare @ 2017-09-28 8:49 UTC (permalink / raw) To: LKML; +Cc: Andrew Morton, Ingo Molnar, Baoquan He 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-09-28 16:01 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-27 8:10 [PATCH] params: Fix an overflow in param_attr_show Jean Delvare 2017-09-27 8:26 ` Ingo Molnar 2017-09-27 9:40 ` Jean Delvare 2017-09-27 13:31 ` Ingo Molnar 2017-09-28 8:02 ` Jean Delvare 2017-09-28 8:11 ` Jean Delvare 2017-09-28 8:38 ` Ingo Molnar 2017-09-28 13:33 ` Jean Delvare 2017-09-28 8:48 ` Ingo Molnar 2017-09-28 8:49 ` Jean Delvare
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.