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]
Date:   Mon, 30 Jan 2017 14:38:03 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Borislav Petkov <bp@...e.de>
Cc:     Rabin Vincent <rabin.vincent@...s.com>, akpm@...ux-foundation.org,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH] printk: fix printk.devkmsg sysctl

On Fri 2017-01-27 19:19:30, Borislav Petkov wrote:
> + 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';

We must not access userspace pointer directly. One solution would be
to use get_user().

But a better solution might be to check also the trailing '\0' in
__control_devkmsg, see below. I has several advantages:

   + we will never assign "invalid" value to @devkmsg_log and
     do not need to revert it. Only devkmsg_log_str must be
     reverted.

   + __control_devkmsg() do not need to return the length of the
     string. We could simply check the error code.

   * the err variable will have the same meaning for both
     __control_devkmsg() and proc_dostring().


Note that

   echo "ratelimitXXXXXX" >/proc/sys/kernel/printk_devkmsg

succeeds because the copied string is limited by DEVKMSG_STR_MAX_SIZE.
We would need to add at least one byte to be able to detect an error
but I am not sure if it is worth it.


Anyway, something like this seems to work:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8b2696420abb..d762190c8c00 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -107,17 +107,18 @@ 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;
+	} else {
+		return -EINVAL;
 	}
-	return -EINVAL;
+
+	return 0;
 }
 
 static int __init control_devkmsg(char *str)
@@ -155,14 +156,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 +172,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;
 }
 
 /*