linux-kernel - [RFC PATCHES] params: Avoid buffer overflow in param_attr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20170919163245.782bd310@endymion>
Date:   Tue, 19 Sep 2017 16:32:45 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     LKML <linux-kernel@...r.kernel.org>
Cc:     Ingo Molnar <mingo@...nel.org>, Baoquan He <bhe@...hat.com>,
        Rusty Russell <rusty@...tcorp.com.au>,
        Dan Streetman <ddstreet@...e.org>,
        Michal Hocko <mhocko@...e.com>
Subject: [RFC PATCHES] params: Avoid buffer overflow in param_attr_show

Hi all,

It was brought to my attention by a customer that 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
doesn't look 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.)

One way to fix it would be to modify the scnprintf calls in
kernel/params.c to consider the buffer size as PAGE_SIZE - 1:

--- linux-4.13.orig/kernel/params.c	2017-09-19 16:20:02.965748249 +0200
+++ linux-4.13/kernel/params.c	2017-09-19 16:20:47.723374030 +0200
@@ -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 - 1, format,		\
 				*((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 - 1, "%s", *((char **)kp->arg));
 }
 EXPORT_SYMBOL(param_get_charp);
 

This guarantees that we always have enough room to add the trailing
'\n'.

However my impression is that seq_file doesn't care about strings being
NUL-terminated, all it sees is a buffer and a byte count. So we might
as well just replace the trailing '\0' with a '\n' and not add another
'\0' after that. This nicely avoids the buffer overflow, and is also
faster than calling strcat():

--- linux-4.13.orig/kernel/params.c	2017-07-03 01:07:02.000000000 +0200
+++ linux-4.13/kernel/params.c	2017-09-19 15:20:28.870798377 +0200
@@ -550,8 +550,8 @@ static ssize_t param_attr_show(struct mo
 	count = attribute->param->ops->get(buf, attribute->param);
 	kernel_param_unlock(mk->mod);
 	if (count > 0) {
-		strcat(buf, "\n");
-		++count;
+		/* Replace the terminating NUL by a newline */
+		buf[count++] = '\n';
 	}
 	return count;
 }

A quick test did not reveal any problem.

Yet another option would be to add the trailing '\n' when writing to
the buffer originally:

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

The only case where it makes a difference is for empty string
parameters. They are rare but they exist (for example parameter
edid_firmware of module drm_kms_helper is an empty string by default.)
Reading such parameters would now return a blank line, instead of
nothing at all. I think it is more consistent, more efficient and more
elegant, but I don't know if anyone is worried about potential side
effects? If so, this special case could also be handled directly in
function param_get_charp.

I would personally prefer the 3rd option, but I'm curious if other
developers have different opinions (and if so, why.)

Thanks,
-- 
Jean Delvare
SUSE L3 Support