[PATCH] printk: fix printk.devkmsg sysctl

[PATCH] printk: fix printk.devkmsg sysctl

[Rabin Vincent]

From: Rabin Vincent <rabinv@axis.com>

The comment says that it doesn't want to accept trailing crap but that's
just what 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

Fix it.

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
 #

As a side effect, the "off\nX" case is not rejected anymore, but that is how
the other sysctl files behave:

 # cat /proc/sys/kernel/printk_ratelimit
 0
 # printf "5\nX" >/proc/sys/kernel/printk_ratelimit
 # cat /proc/sys/kernel/printk_ratelimit
 5

Fixes: 750afe7babd117d ("printk: add kernel parameter to control writes to /dev/kmsg")
Signed-off-by: Rabin Vincent <rabinv@axis.com>
---
 kernel/printk/printk.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8b26964..032b9e0 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -107,15 +107,15 @@ static int __control_devkmsg(char *str)
 	if (!str)
 		return -EINVAL;
 
-	if (!strncmp(str, "on", 2)) {
+	if (!strcmp(str, "on")) {
 		devkmsg_log = DEVKMSG_LOG_MASK_ON;
-		return 2;
-	} else if (!strncmp(str, "off", 3)) {
+		return 0;
+	} else if (!strcmp(str, "off")) {
 		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
-		return 3;
-	} else if (!strncmp(str, "ratelimit", 9)) {
+		return 0;
+	} else if (!strcmp(str, "ratelimit")) {
 		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
-		return 9;
+		return 0;
 	}
 	return -EINVAL;
 }
@@ -177,7 +177,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 		 * Do not accept an unknown string OR a known string with
 		 * trailing crap...
 		 */
-		if (err < 0 || (err + 1 != *lenp)) {
+		if (err < 0) {
 
 			/* ... and restore old setting. */
 			devkmsg_log = old;
-- 
2.1.4

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

[Borislav Petkov]

On Fri, Jan 27, 2017 at 02:11:46PM +0100, Rabin Vincent wrote:
> From: Rabin Vincent <rabinv@axis.com>
> 
> The comment says that it doesn't want to accept trailing crap but that's
> just what it allows:

...

> @@ -177,7 +177,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  		 * Do not accept an unknown string OR a known string with
>  		 * trailing crap...
>  		 */
> -		if (err < 0 || (err + 1 != *lenp)) {

Grr, that's that damn '\n'

echo off > /proc/sys/kernel/printk_devkmsg

works, of course.

Ok, I don't want to relax the strncmp() above and would still like to
return the exact length compared.

So please change the check above to allow the following inputs:

	<str>

or

	<str>\n

I.e., a trailing, *optional*, '\n' is allowed.

This way we're accepting the two most common ways to input strings:

$ echo <str> > ...

and 

$ echo -n <str> > ...

Thanks!

-- 
Regards/Gruss,
    Boris.

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

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

[Rabin Vincent]

On Fri, Jan 27, 2017 at 04:01:41PM +0100, Borislav Petkov wrote:
> On Fri, Jan 27, 2017 at 02:11:46PM +0100, Rabin Vincent wrote:
> > @@ -177,7 +177,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> >  		 * Do not accept an unknown string OR a known string with
> >  		 * trailing crap...
> >  		 */
> > -		if (err < 0 || (err + 1 != *lenp)) {
> 
> Grr, that's that damn '\n'
> 
> echo off > /proc/sys/kernel/printk_devkmsg
> 
> works, of course.
> 
> Ok, I don't want to relax the strncmp() above and would still like to
> return the exact length compared.
> 
> So please change the check above to allow the following inputs:
> 
> 	<str>
> 
> or
> 
> 	<str>\n
> 
> I.e., a trailing, *optional*, '\n' is allowed.

proc_dostring() eats the '\n' and stops after that so we never see it or
what comes after, so we need an strlen():

8<----
>From ec7e02cdf5b6c9fb1492670928bb7ea4386ca87d Mon Sep 17 00:00:00 2001
From: Rabin Vincent <rabinv@axis.com>
Date: Fri, 27 Jan 2017 14:03:07 +0100
Subject: [PATCHv2] printk: fix printk.devkmsg sysctl

The comment says that it doesn't want to accept trailing crap but the
code does allow it:

 # 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

Fix it.

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

Fixes: 750afe7babd117d ("printk: add kernel parameter to control writes to /dev/kmsg")
Signed-off-by: Rabin Vincent <rabinv@axis.com>
---
 kernel/printk/printk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8b26964..935ed71 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -177,7 +177,8 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 		 * Do not accept an unknown string OR a known string with
 		 * trailing crap...
 		 */
-		if (err < 0 || (err + 1 != *lenp)) {
+		if (err < 0 || (err != *lenp && err + 1 != *lenp) ||
+		    err != strlen(devkmsg_log_str)) {
 
 			/* ... and restore old setting. */
 			devkmsg_log = old;
-- 
2.1.4

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

[Borislav Petkov]

+ 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)
-- 

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

[Petr Mladek]

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

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

[Borislav Petkov]

On Mon, Jan 30, 2017 at 02:38:03PM +0100, Petr Mladek wrote:
> We must not access userspace pointer directly. One solution would be
> to use get_user().

Good point.

> 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().
>

Sounds good to me.

> 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.

So my reasoning here was to allow only '<str>' or '<str>\n' - where
<str> is one of the three accepted settings and fail everything else.
That's why I did return the strlen.

Accepting

$ echo "ratelimitXXXXXX" > ...

looks strange to me and silly and misleading and...

IOW, I'd like the user to know what she does and mean it. No sloppy
inputs.

-- 
Regards/Gruss,
    Boris.

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

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

[Rabin Vincent]

On Fri, Jan 27, 2017 at 07:19:30PM +0100, Borislav Petkov wrote:
> 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.

Would it be possible for you to please submit it as a patch yourself so
that this gets fixed in the way you like?  Thank you.

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

[Borislav Petkov]

On Mon, Jan 30, 2017 at 06:31:38PM +0100, Rabin Vincent wrote:
> Would it be possible for you to please submit it as a patch yourself so
> that this gets fixed in the way you like?  Thank you.

Sure. I'll add your Reported-by when we're done.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

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.

:-)

Anyway, more fun tomorrow.

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];
        unsigned int setting;
        int len, err;

        if (!write) {
                err = proc_dostring(table, write, buffer, lenp, ppos);
                if (err)
                        return err;

                return 0;
        }

        if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK)
                return -EINVAL;

        len = min_t(int, (int)*lenp, DEVKMSG_STR_MAX_SIZE);

        memset(tmp_str, 0, DEVKMSG_STR_MAX_SIZE);

        err = copy_from_user(tmp_str, buffer, len);
        if (err)
                return -EFAULT;

        err = __control_devkmsg(tmp_str, &setting);
        if (err < 0)
                return -EINVAL;

        /* known string */
        if (err == len)
                goto assign;

        /* known string with a trailing \n */
        if ((err + 1 == len) && (tmp_str[len - 1] == '\n'))
                goto assign;

        return -EINVAL;

assign:
        if (devkmsg_log != setting) {
                memset(devkmsg_log_str, 0, DEVKMSG_STR_MAX_SIZE);
                strncpy(devkmsg_log_str, tmp_str, err);
                devkmsg_log = setting;
       }

        return 0;
}

---
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8b2696420abb..9bd84ca700d5 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -102,19 +102,19 @@ enum devkmsg_log_masks {
 
 static unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
 
-static int __control_devkmsg(char *str)
+static int __control_devkmsg(char *str, unsigned int *ret)
 {
 	if (!str)
 		return -EINVAL;
 
 	if (!strncmp(str, "on", 2)) {
-		devkmsg_log = DEVKMSG_LOG_MASK_ON;
+		*ret = DEVKMSG_LOG_MASK_ON;
 		return 2;
 	} else if (!strncmp(str, "off", 3)) {
-		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
+		*ret = DEVKMSG_LOG_MASK_OFF;
 		return 3;
 	} else if (!strncmp(str, "ratelimit", 9)) {
-		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
+		*ret = DEVKMSG_LOG_MASK_DEFAULT;
 		return 9;
 	}
 	return -EINVAL;
@@ -122,21 +122,25 @@ static int __control_devkmsg(char *str)
 
 static int __init control_devkmsg(char *str)
 {
-	if (__control_devkmsg(str) < 0)
+	unsigned int setting;
+
+	if (__control_devkmsg(str, &setting) < 0)
 		return 1;
 
 	/*
 	 * Set sysctl string accordingly:
 	 */
-	if (devkmsg_log == DEVKMSG_LOG_MASK_ON) {
+	if (setting == DEVKMSG_LOG_MASK_ON) {
 		memset(devkmsg_log_str, 0, DEVKMSG_STR_MAX_SIZE);
 		strncpy(devkmsg_log_str, "on", 2);
-	} else if (devkmsg_log == DEVKMSG_LOG_MASK_OFF) {
+	} else if (setting == DEVKMSG_LOG_MASK_OFF) {
 		memset(devkmsg_log_str, 0, DEVKMSG_STR_MAX_SIZE);
 		strncpy(devkmsg_log_str, "off", 3);
 	}
 	/* else "ratelimit" which is set by default. */
 
+	devkmsg_log = setting;
+
 	/*
 	 * Sysctl cannot change it anymore. The kernel command line setting of
 	 * this parameter is to force the setting to be permanent throughout the
@@ -154,37 +158,48 @@ char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
 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;
+	char tmp_str[DEVKMSG_STR_MAX_SIZE];
+	unsigned int setting;
+	int len, err;
 
-	if (write) {
-		if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK)
-			return -EINVAL;
+	if (!write) {
+		err = proc_dostring(table, write, buffer, lenp, ppos);
+		if (err)
+			return err;
 
-		old = devkmsg_log;
-		strncpy(old_str, devkmsg_log_str, DEVKMSG_STR_MAX_SIZE);
+		return 0;
 	}
 
-	err = proc_dostring(table, write, buffer, lenp, ppos);
+	if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK)
+		return -EINVAL;
+
+	len = min_t(int, (int)*lenp, DEVKMSG_STR_MAX_SIZE);
+
+	memset(tmp_str, 0, DEVKMSG_STR_MAX_SIZE);
+
+	err = copy_from_user(tmp_str, buffer, len);
 	if (err)
-		return err;
+		return -EFAULT;
 
-	if (write) {
-		err = __control_devkmsg(devkmsg_log_str);
+	err = __control_devkmsg(tmp_str, &setting);
+	if (err < 0)
+		return -EINVAL;
 
-		/*
-		 * Do not accept an unknown string OR a known string with
-		 * trailing crap...
-		 */
-		if (err < 0 || (err + 1 != *lenp)) {
+	/* known string */
+	if (err == len)
+		goto assign;
 
-			/* ... and restore old setting. */
-			devkmsg_log = old;
-			strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE);
+	/* known string with a trailing \n */
+	if ((err + 1 == len) && (tmp_str[len - 1] == '\n'))
+		goto assign;
 
-			return -EINVAL;
-		}
+	return -EINVAL;
+
+assign:
+	if (devkmsg_log != setting) {
+		memset(devkmsg_log_str, 0, DEVKMSG_STR_MAX_SIZE);
+		strncpy(devkmsg_log_str, tmp_str, err);
+		devkmsg_log = setting;
 	}
 
 	return 0;

-- 
Regards/Gruss,
    Boris.

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

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

[Petr Mladek]

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

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

[Sergey Senozhatsky]

Hello,

sorry, catching up with the emails.

there seems to be two independent patches. I'll just comment in one
thread.



On (01/31/17 14:55), Petr Mladek wrote:
[..]
> 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
> 

for these cases (echo/echo -n/echo -e/etc. etc.) I usually use
sysfs_streq() from lib/string.c

wouldn't sysfs_streq() do the trick here?

	-ss