Re: [PATCH -v4 2/2] printk: Add kernel parameter to control writes to /dev/kmsg

[Steven Rostedt]

On Thu, 14 Jul 2016 13:23:23 -0700
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Fri,  8 Jul 2016 11:18:50 +0200 Borislav Petkov <bp@xxxxxxxxx> wrote:
> 
> > From: Borislav Petkov <bp@xxxxxxx>
> > 
> > Add a "printk.devkmsg" kernel command line parameter which controls how
> > userspace writes into /dev/kmsg. It has three options:
> > 
> > * ratelimit - ratelimit logging from userspace.
> > * on  - unlimited logging from userspace
> > * off - logging from userspace gets ignored
> > 
> > The default setting is to ratelimit the messages written to it.  
> 
> Which changes current kernel behaviour.  Can we please get some
> discussion into this changelog which justifies that decision?  What's
> the reasoning and how do we know it won't break existing stuff?

Well, Linus was the one that suggested the ratelimit default. Usually
when userspace starts spamming dev/kmsg, it breaks the kernel. Well,
it's the same as when anything in the kernel spams the console. Why let
userspace do it (and it does it a lot) where data gets lost anyway, and
worse yet, so do all kernel messages. Which is the complaint about
letting userspace spam the kernel printk buffer in the first place.


> 
> > It additionally does not limit logging to /dev/kmsg while the system is
> > booting if we haven't disabled it on the command line.
> > 
> > This patch is based on previous patches from Linus and Steven.
> > 
> > In addition, we can control the logging from a lower priority
> > sysctl interface - kernel.printk_devkmsg={0,1,2} - with numeric values
> > corresponding to the options above.
> > 
> > That interface will succeed only if printk.devkmsg *hasn't* been supplied
> > on the command line. If it has, then printk.devkmsg is a one-time setting
> > which remains for the duration of the system lifetime.  
> 
> Please also include a description of the reasoning behind this design
> decision.
> 
> > Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Franck Bui <fbui@xxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: Uwe Kleine-K__nig <u.kleine-koenig@xxxxxxxxxxxxxx>  
> 
> Did nobody ack any of this?

Heh, I've been reviewing each of his previous releases. This one got
sorta lost in my inbox since I'm working hard on my other
responsibilities done before I'm off on vacation.

> 
> >  Documentation/kernel-parameters.txt |  6 +++
> >  Documentation/sysctl/kernel.txt     | 14 ++++++
> >  include/linux/printk.h              |  7 +++
> >  kernel/printk/printk.c              | 86 +++++++++++++++++++++++++++++++++----
> >  kernel/sysctl.c                     |  9 ++++
> >  5 files changed, 114 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index 82b42c958d1c..0b1fea56dd49 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -3150,6 +3150,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> >  			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
> >  			default: disabled
> >  
> > +	printk.devkmsg={on,off}
> > +			Control writing to /dev/kmsg.
> > +			on - unlimited logging to /dev/kmsg from userspace
> > +			off - logging to /dev/kmsg disabled
> > +			Default: ratelimited logging.  
> 
> printk.devkmsg=ratelimit is undocumented?

That needs to be fixed.

> 
> >  	printk.time=	Show timing data prefixed to each printk message line
> >  			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
> >  
> > diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> > index a3683ce2a2f3..dec84d90061c 100644
> > --- a/Documentation/sysctl/kernel.txt
> > +++ b/Documentation/sysctl/kernel.txt
> > @@ -752,6 +752,20 @@ send before ratelimiting kicks in.
> >  
> >  ==============================================================
> >  
> > +printk_devkmsg:
> > +
> > +Control the logging to /dev/kmsg from userspace:
> > +
> > +0: default, ratelimited
> > +1: unlimited logging to /dev/kmsg from userspace
> > +2: logging to /dev/kmsg disabled  
> 
> hm.  If we're going to make this a pain old 0/1/2 then perhaps the boot
> parameter should also simply accept 0/1/2.

Good ponit.

> 
> > +The kernel command line parameter printk.devkmsg= overrides this and is
> > +a one-time setting until next reboot: once set, it cannot be changed by
> > +this sysctl interface anymore.  
> 
> Why?

Should be added that we don't trust userspace. If someone wants to
disable userspace from touching /dev/kmsg, then if it is on the kernel
command line, then it should be permanent. I'm sick of userspace
using the kernel printk buffers for their own messaging. It makes it
harder to debug kernel issues. Thus, if /dev/kmsg is disabled on boot
up, I don't want any userspace application to have the ability to
're-enable' it again.


> 
> > +
> > +==============================================================
> > +
> >  randomize_va_space:
> >  
> >  This option can be used to select the type of process address
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index f4da695fd615..e6bb50751504 100644
> > --- a/include/linux/printk.h
> > +++ b/include/linux/printk.h
> > @@ -171,6 +171,13 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
> >  extern int printk_delay_msec;
> >  extern int dmesg_restrict;
> >  extern int kptr_restrict;
> > +extern unsigned int devkmsg_log;
> > +
> > +struct ctl_table;  
> 
> Please put these forward declarations near top-of-file.  Otherwise we
> can later get duplicates.
> 
> > +extern int
> > +devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void __user *buf,
> > +			  size_t *lenp, loff_t *ppos);
> >  
> >  extern void wake_up_klogd(void);
> >
> > ...
> >
> > +unsigned int __read_mostly devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> > +static int __init control_devkmsg(char *str)
> > +{
> > +	if (!str)
> > +		return -EINVAL;
> > +
> > +	if (!strncmp(str, "on", 2))
> > +		devkmsg_log = DEVKMSG_LOG_MASK_ON;
> > +	else if (!strncmp(str, "off", 3))
> > +		devkmsg_log = DEVKMSG_LOG_MASK_OFF;
> > +	else if (!strncmp(str, "ratelimit", 9))
> > +		devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT;
> > +	else
> > +		return -EINVAL;
> > +
> > +	/* Sysctl cannot change it anymore. */  
> 
> Please enhance the comment to describe why this is being done.  The
> reasoning behind it.

Yep, need to add: "The kernel command line setting of this parameter is
to force the setting to be permanent throughout the runtime of the
system and can not be changed at a later time. Used by admins that
don't trust their systems to behave properly."

;-)

-- Steve

> 
> > +	devkmsg_log |= DEVKMSG_LOG_MASK_LOCK;
> > +
> > +	return 0;
> > +}
> > +__setup("printk.devkmsg=", control_devkmsg);
> > +
> > +
> > +int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> > +			      void __user *buffer, size_t *lenp, loff_t *ppos)
> > +{
> > +	if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK) {
> > +		if (write)
> > +			return -EINVAL;
> > +	}
> > +
> > +	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> > +}
> > +
> >
> > ...
> >