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

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]

Message-ID: <20160705174749.351d77d1@gandalf.local.home>
Date:	Tue, 5 Jul 2016 17:47:49 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Borislav Petkov <bp@...en8.de>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Franck Bui <fbui@...e.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH -v3 2/2] printk: Add kernel parameter to control writes
 to /dev/kmsg

On Mon,  4 Jul 2016 16:24:52 +0200
Borislav Petkov <bp@...en8.de> wrote:

> @@ -614,6 +663,7 @@ struct devkmsg_user {
>  	u64 seq;
>  	u32 idx;
>  	enum log_flags prev;
> +	struct ratelimit_state rs;
>  	struct mutex lock;
>  	char buf[CONSOLE_EXT_LOG_MAX];
>  };
> @@ -623,11 +673,24 @@ static ssize_t devkmsg_write(struct kiocb *iocb, struct iov_iter *from)
>  	char *buf, *line;
>  	int level = default_message_loglevel;
>  	int facility = 1;	/* LOG_USER */
> +	struct file *file = iocb->ki_filp;
> +	struct devkmsg_user *user = file->private_data;
>  	size_t len = iov_iter_count(from);
>  	ssize_t ret = len;
>  
> -	if (len > LOG_LINE_MAX)
> +	if (!user || len > LOG_LINE_MAX)
>  		return -EINVAL;
> +
> +	/* Ignore when user logging is disabled. */
> +	if (devkmsg_log & DEVKMSG_LOG_MASK_OFF)
> +		return len;

I wonder if we should return some sort of error message here? ENODEV?

> +
> +	/* Ratelimit when not explicitly enabled or when we're not booting. */
> +	if ((system_state != SYSTEM_BOOTING) && !(devkmsg_log & DEVKMSG_LOG_MASK_ON)) {
> +		if (!___ratelimit(&user->rs, current->comm))
> +			return ret;
> +	}
> +
>  	buf = kmalloc(len+1, GFP_KERNEL);
>  	if (buf == NULL)
>  		return -ENOMEM;
> @@ -801,18 +864,20 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	int err;
>  
>  	/* write-only does not need any file context */
> -	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
> -		return 0;
> -
> -	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> -				       SYSLOG_FROM_READER);
> -	if (err)
> -		return err;
> +	if ((file->f_flags & O_ACCMODE) != O_WRONLY) {
> +		err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +					       SYSLOG_FROM_READER);
> +		if (err)
> +			return err;
> +	}

Hmm, there's no error message when it is disabled? I'm not sure that is
what we want. I specifically had the return be an error on open if it
was disabled, because (surprisingly) systemd does the right thing and
uses another utility for syslogging.

If you silently fail here, then we lose all logging because systemd
thinks this is working when it is not. That's not what I want.

-- Steve

>  
>  	user = kmalloc(sizeof(struct devkmsg_user), GFP_KERNEL);
>  	if (!user)
>  		return -ENOMEM;
>  
> +	ratelimit_default_init(&user->rs);
> +	ratelimit_set_flags(&user->rs, RATELIMIT_MSG_ON_RELEASE);
> +
>  	mutex_init(&user->lock);
>  
>  	raw_spin_lock_irq(&logbuf_lock);
> @@ -831,6 +896,8 @@ static int devkmsg_release(struct inode *inode, struct file *file)
>  	if (!user)
>  		return 0;
>  
> +	ratelimit_state_exit(&user->rs);
> +
>  	mutex_destroy(&user->lock);
>  	kfree(user);
>  	return 0;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 87b2fc38398b..013d5fe0636a 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -814,6 +814,15 @@ static struct ctl_table kern_table[] = {
>  		.extra2		= &ten_thousand,
>  	},
>  	{
> +		.procname	= "printk_devkmsg",
> +		.data		= &devkmsg_log,
> +		.maxlen		= sizeof(unsigned int),
> +		.mode		= 0644,
> +		.proc_handler	= devkmsg_sysctl_set_loglvl,
> +		.extra1		= &zero,
> +		.extra2		= &two,
> +	},
> +	{
>  		.procname	= "dmesg_restrict",
>  		.data		= &dmesg_restrict,
>  		.maxlen		= sizeof(int),