params: Fix an overflow in param_attr_show

PATCH In review Jean Delvare 2 participants 10 messages Sep 27, 2017 – Sep 28, 2017

Share & tools

Jean Delvare

author

2017-09-27 08:10 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 kernel/params.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

kernel/params.c +9 ~~-13~~

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

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Ingo Molnar

2017-09-27 08:26 UTC #1 lore raw

Quoted 54 lines · Jean Delvare

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 <[email protected]>

---
 kernel/params.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

kernel/params.c +9 -9

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

kernel/params.c static ssize_t param_attr_show(struct mo · line 553

Quoted 9 lines

kernel/params.c +0 -4

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

kernel/params.c char *parse_args(const char *doing, · line 241

Quoted 3 lines

kernel/params.c +3 -0

+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

Jean Delvare

author

2017-09-27 09:40 UTC #2 lore raw

Hi Ingo,

Quoted 69 lines · Ingo Molnar

Quoted 54 lines · Jean Delvare
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 <[email protected]>
---
 kernel/params.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
kernel/params.c +9 -9
@@ -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);
 
kernel/params.c static ssize_t param_attr_show(struct mo · line 553
Quoted 9 lines
kernel/params.c +0 -4
@@ -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
Quoted 1 line

Quoted 6 lines

kernel/params.c char *parse_args(const char *doing, · line 241
Quoted 3 lines
kernel/params.c +3 -0
+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?

Quoted 6 lines

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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Ingo Molnar

2017-09-27 13:31 UTC #3 lore raw

Quoted 49 lines · Jean Delvare

Quoted 42 lines

Quoted 29 lines

kernel/params.c +9 -9

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

kernel/params.c static ssize_t param_attr_show(struct mo · line 553

Quoted 9 lines

kernel/params.c +0 -4

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

Quoted 1 line

Quoted 6 lines

kernel/params.c char *parse_args(const char *doing, · line 241
Quoted 3 lines
kernel/params.c +3 -0
+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.

Quoted 2 lines

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.

Quoted 9 lines

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

Quoted 6 lines

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.

Quoted 2 lines

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

Jean Delvare

author

2017-09-28 08:02 UTC #4 lore raw

Quoted 10 lines · Ingo Molnar

Quoted 9 lines · Jean Delvare
Quoted 8 lines
So the \n additions to the STANDARD_PARAM_DEF() lines
kernel/params.c char *parse_args(const char *doing, · line 241
Quoted 3 lines
kernel/params.c +3 -0
+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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 14 lines

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

Quoted 8 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);
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...

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Jean Delvare

author

2017-09-28 08:11 UTC #5 lore raw

Quoted 20 lines · Jean Delvare

Quoted 11 lines · Ingo Molnar
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 ;-)

Quoted 4 lines

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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Ingo Molnar

Acked-by

2017-09-28 08:38 UTC #6 lore raw

Quoted 21 lines · Jean Delvare

Quoted 20 lines · Jean Delvare
Quoted 11 lines · Ingo Molnar
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 ;-)

Quoted 8 lines

Quoted 4 lines
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 <[email protected]>

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

Thanks,

Ingo

Jean Delvare

author

2017-09-28 16:01 UTC #7 lore raw

Quoted 10 lines · Ingo Molnar

Quoted 3 lines · Jean Delvare
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 <[email protected]>
Note that the vertical alignment makes things easier to read regardless of the \n.

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

Signature · 2 lines

Jean Delvare
SUSE L3 Support

Ingo Molnar

2017-09-28 08:48 UTC #8 lore raw

In reply to Jean Delvare

Quoted 11 lines · Jean Delvare

Quoted 8 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);
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.

Quoted 5 lines

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:

Quoted 4 lines

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

Jean Delvare

author

2017-09-28 08:49 UTC #9 lore raw

In reply to Jean Delvare

Quoted 12 lines · Jean Delvare

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.

Quoted 7 lines

Credits to Teradata for discovering this issue.
Signed-off-by Jean Delvare <[email protected]>
---
 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.

Signature · 2 lines

Jean Delvare
SUSE L3 Support