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