linux-kernel - Re: [PATCH] printk: fix printk.devkmsg sysctl

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170127181930.sshkgcocttbwu7k4@pd.tnic>
Date:   Fri, 27 Jan 2017 19:19:30 +0100
From:   Borislav Petkov <bp@...e.de>
To:     Rabin Vincent <rabin.vincent@...s.com>
Cc:     akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH] printk: fix printk.devkmsg sysctl

+ printk folk.

On Fri, Jan 27, 2017 at 04:42:30PM +0100, Rabin Vincent wrote:
> proc_dostring() eats the '\n' and stops

Not a problem, see diff below.

Please do it this way instead because after a month no one will remember
what this complex conditional means

> +             if (err < 0 || (err != *lenp && err + 1 != *lenp) ||
> +                 err != strlen(devkmsg_log_str)) {

and why we did it this way.

Also, having the different parts of the conditional explained with a
comment makes it much more obvious.

Also,

> Before this patch:
> 
>  # cat /proc/sys/kernel/printk_devkmsg
>  ratelimit
>  # echo off > /proc/sys/kernel/printk_devkmsg
>  # sysctl -w kernel.printk_devkmsg=off
>  sysctl: short write
>  # echo -n off > /proc/sys/kernel/printk_devkmsg
>  -sh: echo: write error: Invalid argument
>  # echo -n offX > /proc/sys/kernel/printk_devkmsg
>  #
>  # printf "off\nX" >/proc/sys/kernel/printk_devkmsg
>  -sh: printf: write error: Invalid argument
> 
> After this patch:
> 
>  # cat /proc/sys/kernel/printk_devkmsg
>  ratelimit
>  # echo off > /proc/sys/kernel/printk_devkmsg
>  # sysctl -w kernel.printk_devkmsg=off
>  kernel.printk_devkmsg = off
>  # echo -n off > /proc/sys/kernel/printk_devkmsg
>  # echo -n offX > /proc/sys/kernel/printk_devkmsg
>  -sh: echo: write error: Invalid argument
>  # printf "off\nX" >/proc/sys/kernel/printk_devkmsg
>  -sh: printf: write error: Invalid argument

you can leave all those printk examples in the commit message but you
should also explain why we're doing what we're doing. To allow this and
that input and disallow others and why we need to "fish out" newline
before proc_dostring(), yadda yadda.

Thanks.

---

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8b2696420abb..2a1f7c8efb16 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -156,14 +156,19 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 {
 	char old_str[DEVKMSG_STR_MAX_SIZE];
 	unsigned int old;
+	bool newline = false;
 	int err;
 
 	if (write) {
+		char __user *b = buffer;
+
 		if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK)
 			return -EINVAL;
 
 		old = devkmsg_log;
 		strncpy(old_str, devkmsg_log_str, DEVKMSG_STR_MAX_SIZE);
+
+		newline = b[*lenp - 1] == '\n';
 	}
 
 	err = proc_dostring(table, write, buffer, lenp, ppos);
@@ -173,21 +178,27 @@ 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)) {
+		/* Do not accept an unknown string... */
+		if (err < 0)
+			goto restore;
 
-			/* ... and restore old setting. */
-			devkmsg_log = old;
-			strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
+		/* ... known string without trailing '\n' is fine ... */
+		if (err == *lenp)
+			return 0;
 
-			return -EINVAL;
-		}
+		/* ... so is known string with a trailing '\n'.*/
+		if (err + 1 != *lenp || !newline)
+			goto restore;
 	}
 
 	return 0;
+
+restore:
+	/* Restore old setting. */
+	devkmsg_log = old;
+	strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
+
+	return -EINVAL;
 }
 
 /*
---

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--