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.