Re: [PATCH] printk: fix printk.devkmsg sysctl

[Petr Mladek <pmladek@suse.com>]

On Mon 2017-01-30 23:39:12, Borislav Petkov wrote:
> On Mon, Jan 30, 2017 at 06:03:18PM +0100, Borislav Petkov wrote:
> > IOW, I'd like the user to know what she does and mean it. No sloppy
> > inputs.
> 
> Ok, here's what I wanna do. I decided to do my own parsing on the write
> path since proc_dostring() does not really allow me to look at the input
> string. Here's what I have so far, I'll do some more testing tomorrow.
> 
> I know, the diff is practically unreadable so let me paste the whole
> function here.
> 
> So this way I can really control which input is accepted and which not
> without jumping through hoops and doing silly games with the length.
>
> And of course I'm not saving/restoring strings like a crazy person.

This solution is cleaner. I am only afraid that we might miss some
subtle sysctl specialities. For example, these functions seems
to have some special handling of empty values:

static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
[...]
{
[...]
	if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
		*lenp = 0;
		return 0;
	}

And more importantly. There seems to be some strict write mode, see
SYSCTL_WRITES_STRICT, SYSCTL_WRITES_WARN.

Also we might need to shift *ppos by the length of written string.

The original code got all these things for free from proc_dostring().


> int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>                               void __user *buffer, size_t *lenp, loff_t *ppos)
> {
>         char tmp_str[DEVKMSG_STR_MAX_SIZE];
[...]
>         len = min_t(int, (int)*lenp, DEVKMSG_STR_MAX_SIZE);
[...]
>         err = copy_from_user(tmp_str, buffer, len);
>         if (err)
>                 return -EFAULT;

The code still reads only 10 bytes and could get cheated ;-)
For example, I get:

$> echo on > /proc/sys/kernel/printk_devkmsg
$> cat /proc/sys/kernel/printk_devkmsg
on
$> echo -e "ratelimit\nXXXX" > /proc/sys/kernel/printk_devkmsg 
-bash: echo: write error: Invalid argument
$> cat /proc/sys/kernel/printk_devkmsg
ratelimit

The second write returns error but the value is written.
I am not sure where the error comes from. It seems
that devkmsg_sysctl_set_loglvl() returns 0 in both cases.


I wonder if my solution still would be a good deal. We could
fix the remaining issue just by increasing the input buffer by
one byte.

Note that proc_dostring() checks for '\n'. It is the only character
that it does not writte into the buffer. Then the extended strncmp()
check will do exact match and find any other mispelling, including
a non-newline suffix.


>From 0ce6125caf314270cb48202390d8a0938fdf316e Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Tue, 31 Jan 2017 12:00:13 +0100
Subject: [PATCH] printk: Fix printk.devkmsg sysctl

The code handling write into /proc/sys/kernel/printk_devkmsg expects
a new line at the end of the string but does not check it. As a result
it allows:

 # echo -n offX > /proc/sys/kernel/printk_devkmsg
 #

while at the same time it rejects legitimate uses:

 # echo -n off > /proc/sys/kernel/printk_devkmsg
 -sh: echo: write error: Invalid argument

This patch keeps using proc_dostring() to avoid a lot of duplicate code.
It increases the buffer size and allows to read one more character
beyond the string "ratelimit". In addition, it increases the limit
for strncmp() so that it checks also the trailing '\0' and do exact
matches.

Note that proc_dostring() checks for the trailing '\n' and does
not write it into the buffer.

As a result __control_devkmsg() is able to detect all misspelling.
and could return proper error codes. Also it never sets a wrong
devkmsg_log so that it need not be reverted later.

The code still does the ugly devkmsg_log_str write/restore.
But it looks like an acceptable deal.

Reported-by: Rabin Vincent <rabin.vincent@axis.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/printk.h |  4 ++--
 kernel/printk/printk.c | 33 +++++++++++----------------------
 2 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 3472cc6b7a60..c5aa0e268990 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -77,8 +77,8 @@ static inline void console_verbose(void)
 		console_loglevel = CONSOLE_LOGLEVEL_MOTORMOUTH;
 }
 
-/* strlen("ratelimit") + 1 */
-#define DEVKMSG_STR_MAX_SIZE 10
+/* strlen("ratelimit") + '\0' + one character to detect wrong entry */
+#define DEVKMSG_STR_MAX_SIZE 11
 extern char devkmsg_log_str[];
 struct ctl_table;
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8b2696420abb..7c56742a8baa 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -107,17 +107,17 @@ static int __control_devkmsg(char *str)
 	if (!str)
 		return -EINVAL;
 
-	if (!strncmp(str, "on", 2)) {
+	/* Do exact match by comparing also the trailing '\0'. */
+	if (!strncmp(str, "on", 3))
 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
-		return 2;
-	} else if (!strncmp(str, "off", 3)) {
+	else if (!strncmp(str, "off", 4))
 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
-		return 3;
-	} else if (!strncmp(str, "ratelimit", 9)) {
+	else if (!strncmp(str, "ratelimit", 10))
 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
-		return 9;
-	}
-	return -EINVAL;
+	else
+		return -EINVAL;
+
+	return 0;
 }
 
 static int __init control_devkmsg(char *str)
@@ -155,14 +155,12 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 			      void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	char old_str[DEVKMSG_STR_MAX_SIZE];
-	unsigned int old;
 	int err;
 
 	if (write) {
 		if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK)
 			return -EINVAL;
 
-		old = devkmsg_log;
 		strncpy(old_str, devkmsg_log_str, DEVKMSG_STR_MAX_SIZE);
 	}
 
@@ -173,21 +171,12 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 	if (write) {
 		err = __control_devkmsg(devkmsg_log_str);
 
-		/*
-		 * Do not accept an unknown string OR a known string with
-		 * trailing crap...
-		 */
-		if (err < 0 || (err + 1 != *lenp)) {
-
-			/* ... and restore old setting. */
-			devkmsg_log = old;
+		/* Restore old setting when unknown parameter was used. */
+		if (err)
 			strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
-
-			return -EINVAL;
-		}
 	}
 
-	return 0;
+	return err;
 }
 
 /*
-- 
1.8.5.6