Skip to contentSkip to conversation

params: Fix potential buffer overflows

PATCH Applied Jean Delvare 2 participants 5 messages Sep 28, 2017

lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

v2 cover params: Fix potential buffer overflows

[PATCH 1/3] params: Fix the maximum length in param_get_string [PATCH 2/3] params: Fix an overflow in param_attr_show [PATCH 3/3] params: Improve STANDARD_PARAM_DEF readability

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#1 lore raw

PATCH v2 1/3 params: Fix the maximum length in param_get_string

The length parameter of strlcpy() is supposed to reflect the size of the target buffer, not of the source string. Harmless in this case as the buffer is PAGE_SIZE long and the source string is always much shorter than this, but conceptually wrong, so let's fix it.

Signed-off-by Jean Delvare <[email protected]>

---
Changes since v1:
* Patch added

 kernel/params.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/params.c +1 -1
@@ -507,7 +507,7 @@ EXPORT_SYMBOL(param_set_copystring);
 int param_get_string(char *buffer, const struct kernel_param *kp)
 {
 	const struct kparam_string *kps = kp->str;
-	return strlcpy(buffer, kps->string, kps->maxlen);
+	return strlcpy(buffer, kps->string, PAGE_SIZE);
 }
 EXPORT_SYMBOL(param_get_string);
Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#2 lore raw

PATCH v2 2/3 params: Fix an overflow in param_attr_show

In reply to 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]>

---
Changes since v1:
* Add the \n inside the STANDARD_PARAM_DEF macro instead of changing all the callers.
* Handle boolean and string types too.
* Handle parameter arrays properly.

 kernel/params.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)
kernel/params.c +7 -10
@@ -224,7 +224,7 @@ char *parse_args(const char *doing,
 	}								\
 	int param_get_##name(char *buffer, const struct kernel_param *kp) \
 	{								\
-		return scnprintf(buffer, PAGE_SIZE, format,		\
+		return scnprintf(buffer, PAGE_SIZE, format "\n",	\
 				*((type *)kp->arg));			\
 	}								\
 	const struct kernel_param_ops param_ops_##name = {			\

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

@@ -301,7 +301,7 @@ EXPORT_SYMBOL(param_set_bool);
 int param_get_bool(char *buffer, const struct kernel_param *kp)
 {
 	/* Y and N chosen as being relatively non-coder friendly */
-	return sprintf(buffer, "%c", *(bool *)kp->arg ? 'Y' : 'N');
+	return sprintf(buffer, "%c\n", *(bool *)kp->arg ? 'Y' : 'N');
 }
 EXPORT_SYMBOL(param_get_bool);
 

@@ -360,7 +360,7 @@ EXPORT_SYMBOL(param_set_invbool);
 
 int param_get_invbool(char *buffer, const struct kernel_param *kp)
 {
-	return sprintf(buffer, "%c", (*(bool *)kp->arg) ? 'N' : 'Y');
+	return sprintf(buffer, "%c\n", (*(bool *)kp->arg) ? 'N' : 'Y');
 }
 EXPORT_SYMBOL(param_get_invbool);
 

@@ -460,8 +460,9 @@ static int param_array_get(char *buffer,
 	struct kernel_param p = *kp;
 
 	for (i = off = 0; i < (arr->num ? *arr->num : arr->max); i++) {
+		/* Replace \n with comma */
 		if (i)
-			buffer[off++] = ',';
+			buffer[off - 1] = ',';
 		p.arg = arr->elem + arr->elemsize * i;
 		check_kparam_locked(p.mod);
 		ret = arr->ops->get(buffer + off, &p);

@@ -507,7 +508,7 @@ EXPORT_SYMBOL(param_set_copystring);
 int param_get_string(char *buffer, const struct kernel_param *kp)
 {
 	const struct kparam_string *kps = kp->str;
-	return strlcpy(buffer, kps->string, PAGE_SIZE);
+	return scnprintf(buffer, PAGE_SIZE, "%s\n", kps->string);
 }
 EXPORT_SYMBOL(param_get_string);
 

@@ -549,10 +550,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
author
#3 lore raw

PATCH v2 3/3 params: Improve STANDARD_PARAM_DEF readability

In reply to Jean Delvare

Align the parameters passed to STANDARD_PARAM_DEF for clarity.

Suggested-by Ingo Molnar <[email protected]>
Signed-off-by Jean Delvare <[email protected]>

---
Changes since v1:
* Patch added

 kernel/params.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
kernel/params.c +8 -8
@@ -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", 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);
 
 int param_set_charp(const char *val, const struct kernel_param *kp)
 {
Signature · 2 lines
Jean Delvare
SUSE L3 Support
Acked-by on the v2 series
#4 lore raw
In reply to Jean Delvare
Quoted 3 lines · Jean Delvare

[PATCH 1/3] params: Fix the maximum length in param_get_string [PATCH 2/3] params: Fix an overflow in param_attr_show [PATCH 3/3] params: Improve STANDARD_PARAM_DEF readability

It all looks sensible to me, so FWIIW:

Acked-by Ingo Molnar <[email protected]>

(but I have not tested it.)

Thanks,

Ingo