[patch 0/4] Syslog permissions, revised

[patch 0/4] Syslog permissions, revised

[Zack Weinberg]

This patchset revises my attempt from last week to allow running klogd
unprivileged without a root shim.  I believe I have addressed all
outstanding objections: in particular, the privilege model enforced by
SELinux is unchanged (you have to have system__syslog_mod to read
/proc/kmsg).  I have also included some nice refactorings (symbolic
constants for sys_syslog opcodes, that sort of thing) and a few
bugfixes (minor and unlikely to affect any live application, but
still).

I hope that this can be considered for 2.6.19; it is low risk in my
opinion and it would be nice to get this functionality into the hands
of the distributors sooner.

zw

[patch 1/4] Add <linux/klog.h>

[Zack Weinberg]

[-- Attachment #1: add_klog_h.diff --]
[-- Type: text/plain, Size: 5846 bytes --]

This patch introduces <linux/klog.h> with symbolic constants for the
various sys_syslog() opcodes, and changes all in-kernel references to
those opcodes to use the constants.  The header is added to the set of
user/kernel interface headers; there is no #ifdef __KERNEL__ block in
it yet, but there will be in subsequent patches, so I put it in as an
'unifdef' header.

zw

Index: linux-2.6/include/linux/Kbuild
===================================================================
--- linux-2.6.orig/include/linux/Kbuild	2006-11-10 13:36:58.000000000 -0800
+++ linux-2.6/include/linux/Kbuild	2006-11-10 13:37:13.000000000 -0800
@@ -253,6 +253,7 @@
 unifdef-y += kernelcapi.h
 unifdef-y += kernel.h
 unifdef-y += keyboard.h
+unifdef-y += klog.h
 unifdef-y += llc.h
 unifdef-y += loop.h
 unifdef-y += lp.h
Index: linux-2.6/include/linux/klog.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6/include/linux/klog.h	2006-11-10 14:16:35.000000000 -0800
@@ -0,0 +1,26 @@
+#ifndef _LINUX_KLOG_H
+#define _LINUX_KLOG_H
+
+/*
+ * Constants for the first argument to the syslog() system call
+ * (aka klogctl()).  These numbers are part of the user space ABI!
+ */
+enum {
+	KLOG_CLOSE           =  0, /* close log */
+	KLOG_OPEN            =  1, /* open log */
+	KLOG_READ            =  2, /* read from log (klogd) */
+
+	KLOG_READ_HIST       =  3, /* read history of log messages (dmesg) */
+	KLOG_READ_CLEAR_HIST =  4, /* read and clear history */
+	KLOG_CLEAR_HIST      =  5, /* just clear history */
+
+	KLOG_DISABLE_CONSOLE =  6, /* disable printk to console */
+	KLOG_ENABLE_CONSOLE  =  7, /* enable printk to console */
+	KLOG_SET_CONSOLE_LVL =  8, /* set minimum severity of messages to be
+				    * printed to console */
+
+	KLOG_GET_UNREAD      =  9, /* return number of unread characters */
+	KLOG_GET_SIZE        = 10  /* return size of log buffer */
+};
+
+#endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-11-10 13:16:47.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-11-10 14:16:34.000000000 -0800
@@ -32,6 +32,7 @@
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
 #include <linux/jiffies.h>
+#include <linux/klog.h>
 
 #include <asm/uaccess.h>
 
@@ -165,21 +166,7 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-/*
- * Commands to do_syslog:
- *
- * 	0 -- Close the log.  Currently a NOP.
- * 	1 -- Open the log. Currently a NOP.
- * 	2 -- Read from the log.
- * 	3 -- Read all messages remaining in the ring buffer.
- * 	4 -- Read and clear all messages remaining in the ring buffer
- * 	5 -- Clear ring buffer.
- * 	6 -- Disable printk's to console
- * 	7 -- Enable printk's to console
- *	8 -- Set level of messages printed to console
- *	9 -- Return number of unread characters in the log buffer
- *     10 -- Return size of the log buffer
- */
+/* See linux/klog.h for the command numbers passed as the first argument.  */
 int do_syslog(int type, char __user *buf, int len)
 {
 	unsigned long i, j, limit, count;
@@ -192,11 +179,11 @@
 		return error;
 
 	switch (type) {
-	case 0:		/* Close log */
+	case KLOG_CLOSE:
 		break;
-	case 1:		/* Open log */
+	case KLOG_OPEN:
 		break;
-	case 2:		/* Read from log */
+	case KLOG_READ:
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -227,10 +214,10 @@
 		if (!error)
 			error = i;
 		break;
-	case 4:		/* Read/clear last kernel messages */
+	case KLOG_READ_CLEAR_HIST:
 		do_clear = 1;
 		/* FALL THRU */
-	case 3:		/* Read last kernel messages */
+	case KLOG_READ_HIST:
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -283,16 +270,16 @@
 			}
 		}
 		break;
-	case 5:		/* Clear ring buffer */
+	case KLOG_CLEAR_HIST:
 		logged_chars = 0;
 		break;
-	case 6:		/* Disable logging to console */
+	case KLOG_DISABLE_CONSOLE:
 		console_loglevel = minimum_console_loglevel;
 		break;
-	case 7:		/* Enable logging to console */
+	case KLOG_ENABLE_CONSOLE:
 		console_loglevel = default_console_loglevel;
 		break;
-	case 8:		/* Set level of messages printed to console */
+	case KLOG_SET_CONSOLE_LVL:
 		error = -EINVAL;
 		if (len < 1 || len > 8)
 			goto out;
@@ -301,10 +288,10 @@
 		console_loglevel = len;
 		error = 0;
 		break;
-	case 9:		/* Number of chars in the log buffer */
+	case KLOG_GET_UNREAD:
 		error = log_end - log_start;
 		break;
-	case 10:	/* Size of the log buffer */
+	case KLOG_GET_SIZE:
 		error = log_buf_len;
 		break;
 	default:
Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-11-10 14:16:41.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-11-10 14:22:56.000000000 -0800
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/poll.h>
 #include <linux/fs.h>
+#include <linux/klog.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -21,27 +22,28 @@
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(1,NULL,0);
+	return do_syslog(KLOG_OPEN,NULL,0);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(0,NULL,0);
+	(void) do_syslog(KLOG_CLOSE,NULL,0);
 	return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	if ((file->f_flags & O_NONBLOCK) && !do_syslog(9, NULL, 0))
+	if ((file->f_flags & O_NONBLOCK)
+	    && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
 		return -EAGAIN;
-	return do_syslog(2, buf, count);
+	return do_syslog(KLOG_READ, buf, count);
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
 	poll_wait(file, &log_wait, wait);
-	if (do_syslog(9, NULL, 0))
+	if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
 		return POLLIN | POLLRDNORM;
 	return 0;
 }

--

[patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

[-- Attachment #1: map_perms.diff --]
[-- Type: text/plain, Size: 6250 bytes --]

As suggested by Stephen Smalley: map the various sys_syslog operations
to a smaller set of privilege codes before calling security modules.
This patch changes the security module interface!  There should be no
change in the actual security semantics enforced by dummy, capability,
nor SELinux (with one exception, clearly marked in sys_syslog).

zw


Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-11-10 13:48:52.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-11-10 14:08:57.000000000 -0800
@@ -23,4 +23,16 @@
 	KLOG_GET_SIZE        = 10  /* return size of log buffer */
 };
 
+#ifdef __KERNEL__
+
+/*
+ * Constants passed by do_syslog to security_syslog to indicate which
+ * privilege is requested.
+ */
+#define KLOGSEC_READ	  0  /* read current messages (klogd) */
+#define KLOGSEC_READHIST  1  /* read message history (dmesg) */
+#define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
+#define KLOGSEC_CONSOLE   3  /* set console log level */
+
+#endif /* __KERNEL__ */
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-11-10 13:44:16.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-11-10 13:54:53.000000000 -0800
@@ -166,6 +166,12 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
+#define security_syslog_or_fail(type) do {		\
+		int error = security_syslog(type);	\
+		if (error)				\
+			return error;			\
+	} while (0)
+
 /* See linux/klog.h for the command numbers passed as the first argument.  */
 int do_syslog(int type, char __user *buf, int len)
 {
@@ -174,16 +180,15 @@
 	char c;
 	int error = 0;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case KLOG_CLOSE:
+		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_OPEN:
+		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_READ:
+		security_syslog_or_fail(KLOGSEC_READ);
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -215,9 +220,11 @@
 			error = i;
 		break;
 	case KLOG_READ_CLEAR_HIST:
+		security_syslog_or_fail(KLOGSEC_CLEARHIST);
 		do_clear = 1;
 		/* FALL THRU */
 	case KLOG_READ_HIST:
+		security_syslog_or_fail(KLOGSEC_READHIST);
 		error = -EINVAL;
 		if (!buf || len < 0)
 			goto out;
@@ -271,15 +278,19 @@
 		}
 		break;
 	case KLOG_CLEAR_HIST:
+		security_syslog_or_fail(KLOGSEC_CLEARHIST);
 		logged_chars = 0;
 		break;
 	case KLOG_DISABLE_CONSOLE:
+		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		console_loglevel = minimum_console_loglevel;
 		break;
 	case KLOG_ENABLE_CONSOLE:
+		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		console_loglevel = default_console_loglevel;
 		break;
 	case KLOG_SET_CONSOLE_LVL:
+		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		error = -EINVAL;
 		if (len < 1 || len > 8)
 			goto out;
@@ -289,9 +300,18 @@
 		error = 0;
 		break;
 	case KLOG_GET_UNREAD:
+		security_syslog_or_fail(KLOGSEC_READ);
 		error = log_end - log_start;
 		break;
 	case KLOG_GET_SIZE:
+		/* This one is allowed if you have _either_
+		   KLOGSEC_READ or KLOGSEC_READHIST.  */
+		error = security_syslog(KLOGSEC_READ);
+		if (error)
+			error = security_syslog(KLOGSEC_READHIST);
+		if (error)
+			break;
+
 		error = log_buf_len;
 		break;
 	default:
Index: linux-2.6/security/commoncap.c
===================================================================
--- linux-2.6.orig/security/commoncap.c	2006-11-10 13:55:49.000000000 -0800
+++ linux-2.6/security/commoncap.c	2006-11-10 13:56:35.000000000 -0800
@@ -23,6 +23,7 @@
 #include <linux/ptrace.h>
 #include <linux/xattr.h>
 #include <linux/hugetlb.h>
+#include <linux/klog.h>
 
 int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
 {
@@ -311,7 +312,7 @@
 
 int cap_syslog (int type)
 {
-	if ((type != 3 && type != 10) && !capable(CAP_SYS_ADMIN))
+	if (type != KLOGSEC_READHIST && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/dummy.c
===================================================================
--- linux-2.6.orig/security/dummy.c	2006-11-10 13:55:49.000000000 -0800
+++ linux-2.6/security/dummy.c	2006-11-10 13:57:22.000000000 -0800
@@ -28,6 +28,7 @@
 #include <linux/hugetlb.h>
 #include <linux/ptrace.h>
 #include <linux/file.h>
+#include <linux/klog.h>
 
 static int dummy_ptrace (struct task_struct *parent, struct task_struct *child)
 {
@@ -96,11 +97,10 @@
 
 static int dummy_syslog (int type)
 {
-	if ((type != 3 && type != 10) && current->euid)
+	if (type != KLOGSEC_READHIST && current->euid)
 		return -EPERM;
 	return 0;
 }
-
 static int dummy_settime(struct timespec *ts, struct timezone *tz)
 {
 	if (!capable(CAP_SYS_TIME))
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2006-11-10 13:55:49.000000000 -0800
+++ linux-2.6/security/selinux/hooks.c	2006-11-10 14:00:49.000000000 -0800
@@ -71,6 +71,7 @@
 #include <linux/string.h>
 #include <linux/selinux.h>
 #include <linux/mutex.h>
+#include <linux/klog.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -1506,25 +1507,17 @@
 		return rc;
 
 	switch (type) {
-		case 3:         /* Read last kernel messages */
-		case 10:        /* Return size of the log buffer */
-			rc = task_has_system(current, SYSTEM__SYSLOG_READ);
-			break;
-		case 6:         /* Disable logging to console */
-		case 7:         /* Enable logging to console */
-		case 8:		/* Set level of messages printed to console */
-			rc = task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
-			break;
-		case 0:         /* Close log */
-		case 1:         /* Open log */
-		case 2:         /* Read from log */
-		case 4:         /* Read/clear last kernel messages */
-		case 5:         /* Clear ring buffer */
-		default:
-			rc = task_has_system(current, SYSTEM__SYSLOG_MOD);
-			break;
+	case KLOGSEC_READHIST:
+		return task_has_system(current, SYSTEM__SYSLOG_READ);
+
+	case KLOGSEC_CONSOLE:
+		return task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
+
+	case KLOGSEC_READ:
+	case KLOGSEC_CLEARHIST:
+	default:
+		return task_has_system(current, SYSTEM__SYSLOG_MOD);
 	}
-	return rc;
 }
 
 /*

--

Re: [patch 2/4] permission mapping for sys_syslog operations

[Arjan van de Ven]

On Sun, 2006-11-12 at 22:40 -0800, Zack Weinberg wrote:
> plain text document attachment (map_perms.diff)
> As suggested by Stephen Smalley: map the various sys_syslog operations
> to a smaller set of privilege codes before calling security modules.
> This patch changes the security module interface!  There should be no
> change in the actual security semantics enforced by dummy, capability,
> nor SELinux (with one exception, clearly marked in sys_syslog).
> 
> zw
> 
> 
> Index: linux-2.6/include/linux/klog.h
> ===================================================================
> --- linux-2.6.orig/include/linux/klog.h	2006-11-10 13:48:52.000000000 -0800
> +++ linux-2.6/include/linux/klog.h	2006-11-10 14:08:57.000000000 -0800
> @@ -23,4 +23,16 @@
>  	KLOG_GET_SIZE        = 10  /* return size of log buffer */
>  };
>  
> +#ifdef __KERNEL__
> +
> +/*
> + * Constants passed by do_syslog to security_syslog to indicate which
> + * privilege is requested.
> + */
> +#define KLOGSEC_READ	  0  /* read current messages (klogd) */
> +#define KLOGSEC_READHIST  1  /* read message history (dmesg) */
> +#define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
> +#define KLOGSEC_CONSOLE   3  /* set console log level */
> +
> +#endif /* __KERNEL__ */

Hi,

you had such a nice userspace/kernel shared header.. and now you mix it
with kernel privates again... can you consider making this a second
header? Eg have a pure userspace clean header and a separate for-kernel
header (which #can include the user header for all I care)... that's
nicer for sure!

Greetings,
   Arjan van de Ven

Re: [patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> you had such a nice userspace/kernel shared header.. and now you mix it
> with kernel privates again... can you consider making this a second
> header?

I thought the point of the "unifdef" thing was that it made a version
of the header with the __KERNEL__ section ripped out, for copying into
/usr/include, so you didn't have to do that ...

zw

Re: [patch 2/4] permission mapping for sys_syslog operations

[Arjan van de Ven]

On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > you had such a nice userspace/kernel shared header.. and now you mix it
> > with kernel privates again... can you consider making this a second
> > header?
> 
> I thought the point of the "unifdef" thing was that it made a version
> of the header with the __KERNEL__ section ripped out, for copying into
> /usr/include, so you didn't have to do that ...

yes it is, however it's mostly for existing stuff/seamless transition.
It's a hack :)
If you can avoid it lets do so; you already have the nice clean header,
so lets not go backwards... you HAVE the clean separation. 

> 
> zw
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [patch 2/4] permission mapping for sys_syslog operations

[Zack Weinberg]

On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> > I thought the point of the "unifdef" thing was that it made a version
> > of the header with the __KERNEL__ section ripped out, for copying into
> > /usr/include, so you didn't have to do that ...
>
> yes it is, however it's mostly for existing stuff/seamless transition.
> It's a hack :)
> If you can avoid it lets do so; you already have the nice clean header,
> so lets not go backwards... you HAVE the clean separation.

ok, but I gotta ask that you tell me what to name the internal header,
I can't think of anything that isn't ugly.

zw

Re: [patch 2/4] permission mapping for sys_syslog operations

[Arjan van de Ven]

On Mon, 2006-11-13 at 09:17 -0800, Zack Weinberg wrote:
> On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> > > I thought the point of the "unifdef" thing was that it made a version
> > > of the header with the __KERNEL__ section ripped out, for copying into
> > > /usr/include, so you didn't have to do that ...
> >
> > yes it is, however it's mostly for existing stuff/seamless transition.
> > It's a hack :)
> > If you can avoid it lets do so; you already have the nice clean header,
> > so lets not go backwards... you HAVE the clean separation.
> 
> ok, but I gotta ask that you tell me what to name the internal header,
> I can't think of anything that isn't ugly.

klog.h vs klogd.h ? or klog_api.h for the user one ?

(and yes I suck at names even more than you do ;)

> 
> zw
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [patch 2/4] permission mapping for sys_syslog operations

[Alexey Dobriyan]

On Mon, Nov 13, 2006 at 06:22:04PM +0100, Arjan van de Ven wrote:
> On Mon, 2006-11-13 at 09:17 -0800, Zack Weinberg wrote:
> > On 11/13/06, Arjan van de Ven <arjan@infradead.org> wrote:
> > > On Mon, 2006-11-13 at 01:29 -0800, Zack Weinberg wrote:
> > > > I thought the point of the "unifdef" thing was that it made a version
> > > > of the header with the __KERNEL__ section ripped out, for copying into
> > > > /usr/include, so you didn't have to do that ...
> > >
> > > yes it is, however it's mostly for existing stuff/seamless transition.
> > > It's a hack :)
> > > If you can avoid it lets do so; you already have the nice clean header,
> > > so lets not go backwards... you HAVE the clean separation.
> >
> > ok, but I gotta ask that you tell me what to name the internal header,
> > I can't think of anything that isn't ugly.
>
> klog.h vs klogd.h ? or klog_api.h for the user one ?
>
> (and yes I suck at names even more than you do ;)

What about security.h, so SELinux folks won't feel lonely there?
:)

[patch 3/4] Refactor do_syslog interface

[Zack Weinberg]

[-- Attachment #1: break_up_do_syslog.diff --]
[-- Type: text/plain, Size: 11066 bytes --]

This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were with one exception: neither
kmsg_close() nor sys_syslog(KLOG_CLOSE, ...) calls security_syslog
at all anymore (close operations should never fail).

Finally, I fixed a couple of minor bugs.  __put_user error handling in
klog_read was slightly off: if __put_user returns an error, that
character should not be consumed from the kernel buffer; if it returns
an error after some characters have already been copied successfully,
the read operation should return the count of already-copied
characters, not the error code.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce that.

zw

Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-11-10 14:59:38.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-11-11 21:52:08.000000000 -0800
@@ -9,43 +9,41 @@
 #include <linux/errno.h>
 #include <linux/time.h>
 #include <linux/kernel.h>
-#include <linux/poll.h>
 #include <linux/fs.h>
 #include <linux/klog.h>
+#include <linux/security.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
-extern wait_queue_head_t log_wait;
-
-extern int do_syslog(int type, char __user *bug, int count);
-
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(KLOG_OPEN,NULL,0);
+	int error = security_syslog(KLOGSEC_READ);
+	if (error)
+		return error;
+	return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(KLOG_CLOSE,NULL,0);
 	return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	if ((file->f_flags & O_NONBLOCK)
-	    && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return -EAGAIN;
-	return do_syslog(KLOG_READ, buf, count);
+	int error = security_syslog(KLOGSEC_READ);
+	if (error)
+		return error;
+	return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-	poll_wait(file, &log_wait, wait);
-	if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return POLLIN | POLLRDNORM;
-	return 0;
+	int error = security_syslog(KLOGSEC_READ);
+	if (error)
+		return error;
+	return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-11-10 14:59:46.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-11-11 21:55:34.000000000 -0800
@@ -17,22 +17,31 @@
 	KLOG_DISABLE_CONSOLE =  6, /* disable printk to console */
 	KLOG_ENABLE_CONSOLE  =  7, /* enable printk to console */
 	KLOG_SET_CONSOLE_LVL =  8, /* set minimum severity of messages to be
-				    * printed to console */
+				      printed to console */
 
 	KLOG_GET_UNREAD      =  9, /* return number of unread characters */
-	KLOG_GET_SIZE        = 10  /* return size of log buffer */
+	KLOG_GET_SIZE        = 10, /* return size of log buffer */
+	KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+				      -- new in 2.6.19 */
 };
 
 #ifdef __KERNEL__
+#include <linux/poll.h>
 
 /*
- * Constants passed by do_syslog to security_syslog to indicate which
- * privilege is requested.
+ * Constants passed to security_syslog to indicate which privilege is
+ * requested.
  */
 #define KLOGSEC_READ	  0  /* read current messages (klogd) */
 #define KLOGSEC_READHIST  1  /* read message history (dmesg) */
 #define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
 #define KLOGSEC_CONSOLE   3  /* set console log level */
 
+/*
+ * Subfunctions of sys_syslog used from fs/proc/kmsg.c.
+ */
+extern int klog_read(char __user *buf, int len, int block);
+extern unsigned int klog_poll(struct file *, poll_table *);
+
 #endif /* __KERNEL__ */
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-11-10 14:57:32.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-11-11 21:54:17.000000000 -0800
@@ -45,7 +45,7 @@
 #define MINIMUM_CONSOLE_LOGLEVEL 1 /* Minimum loglevel we let people use */
 #define DEFAULT_CONSOLE_LOGLEVEL 7 /* anything MORE serious than KERN_DEBUG */
 
-DECLARE_WAIT_QUEUE_HEAD(log_wait);
+static DECLARE_WAIT_QUEUE_HEAD(log_wait);
 
 int console_printk[4] = {
 	DEFAULT_CONSOLE_LOGLEVEL,	/* console_loglevel */
@@ -166,116 +166,142 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-#define security_syslog_or_fail(type) do {		\
-		int error = security_syslog(type);	\
-		if (error)				\
-			return error;			\
-	} while (0)
+/*
+ * Subfunctions of sys_syslog.  Some are also used from fs/proc/kmsg.c.
+ */
 
-/* See linux/klog.h for the command numbers passed as the first argument.  */
-int do_syslog(int type, char __user *buf, int len)
+int klog_read(char __user *buf, int len, int block)
+{
+	int i, error;
+	char c;
+
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (len == 0)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+	if (!block && log_start - log_end == 0)
+		return -EAGAIN;
+
+	error = wait_event_interruptible(log_wait, (log_start - log_end));
+	if (error)
+		return error;
+
+	i = 0;
+	spin_lock_irq(&logbuf_lock);
+	while (log_start != log_end && i < len) {
+		c = LOG_BUF(log_start);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,buf);
+		if (error)
+			goto out;
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+		log_start++;
+		buf++;
+		i++;
+	}
+	spin_unlock_irq(&logbuf_lock);
+out:
+	if (i > 0)
+		return i;
+	return error;
+}
+
+unsigned int klog_poll(struct file *file, poll_table *wait)
+{
+	poll_wait(file, &log_wait, wait);
+	if (log_end - log_start > 0)
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
+static int klog_readhist(char __user *buf, int len)
 {
 	unsigned long i, j, limit, count;
-	int do_clear = 0;
 	char c;
 	int error = 0;
 
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (!len)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+
+	count = len;
+	if (count > log_buf_len)
+		count = log_buf_len;
+	spin_lock_irq(&logbuf_lock);
+	if (count > logged_chars)
+		count = logged_chars;
+	limit = log_end;
+	/*
+	 * __put_user() could sleep, and while we sleep
+	 * printk() could overwrite the messages
+	 * we try to copy to user space. Therefore
+	 * the messages are copied in reverse. <manfreds>
+	 */
+	for (i = 0; i < count && !error; i++) {
+		j = limit-1-i;
+		if (j + log_buf_len < log_end)
+			break;
+		c = LOG_BUF(j);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,&buf[count-1-i]);
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+	}
+	spin_unlock_irq(&logbuf_lock);
+	if (error)
+		return error;
+	error = i;
+	if (i != count) {
+		int offset = count-error;
+		/* buffer overflow during copy, correct user buffer. */
+		for (i = 0; i < error; i++) {
+			if (__get_user(c,&buf[i+offset]) ||
+			    __put_user(c,&buf[i])) {
+				error = -EFAULT;
+				break;
+			}
+			cond_resched();
+		}
+	}
+	return error;
+}
+
+#define security_syslog_or_fail(type) do {	\
+		error = security_syslog(type);	\
+		if (error)			\
+			return error;		\
+	} while (0)
+
+/* See linux/klog.h for the command numbers passed as the first argument.  */
+asmlinkage long sys_syslog(int type, char __user *buf, int len)
+{
+	int error = 0;
 	switch (type) {
 	case KLOG_CLOSE:
-		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_OPEN:
 		security_syslog_or_fail(KLOGSEC_READ);
 		break;
 	case KLOG_READ:
+	case KLOG_READ_NONBLOCK:
 		security_syslog_or_fail(KLOGSEC_READ);
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		error = wait_event_interruptible(log_wait,
-							(log_start - log_end));
-		if (error)
-			goto out;
-		i = 0;
-		spin_lock_irq(&logbuf_lock);
-		while (!error && (log_start != log_end) && i < len) {
-			c = LOG_BUF(log_start);
-			log_start++;
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,buf);
-			buf++;
-			i++;
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (!error)
-			error = i;
+		error = klog_read(buf, len, type == KLOG_READ);
 		break;
 	case KLOG_READ_CLEAR_HIST:
+		security_syslog_or_fail(KLOGSEC_READHIST);
 		security_syslog_or_fail(KLOGSEC_CLEARHIST);
-		do_clear = 1;
-		/* FALL THRU */
+		error = klog_readhist(buf, len);
+		logged_chars = 0;
+		break;
 	case KLOG_READ_HIST:
 		security_syslog_or_fail(KLOGSEC_READHIST);
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		count = len;
-		if (count > log_buf_len)
-			count = log_buf_len;
-		spin_lock_irq(&logbuf_lock);
-		if (count > logged_chars)
-			count = logged_chars;
-		if (do_clear)
-			logged_chars = 0;
-		limit = log_end;
-		/*
-		 * __put_user() could sleep, and while we sleep
-		 * printk() could overwrite the messages
-		 * we try to copy to user space. Therefore
-		 * the messages are copied in reverse. <manfreds>
-		 */
-		for (i = 0; i < count && !error; i++) {
-			j = limit-1-i;
-			if (j + log_buf_len < log_end)
-				break;
-			c = LOG_BUF(j);
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,&buf[count-1-i]);
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (error)
-			break;
-		error = i;
-		if (i != count) {
-			int offset = count-error;
-			/* buffer overflow during copy, correct user buffer. */
-			for (i = 0; i < error; i++) {
-				if (__get_user(c,&buf[i+offset]) ||
-				    __put_user(c,&buf[i])) {
-					error = -EFAULT;
-					break;
-				}
-				cond_resched();
-			}
-		}
+		error = klog_readhist(buf, len);
 		break;
 	case KLOG_CLEAR_HIST:
 		security_syslog_or_fail(KLOGSEC_CLEARHIST);
@@ -293,7 +319,7 @@
 		security_syslog_or_fail(KLOGSEC_CONSOLE);
 		error = -EINVAL;
 		if (len < 1 || len > 8)
-			goto out;
+			break;
 		if (len < minimum_console_loglevel)
 			len = minimum_console_loglevel;
 		console_loglevel = len;
@@ -318,15 +344,9 @@
 		error = -EINVAL;
 		break;
 	}
-out:
 	return error;
 }
 
-asmlinkage long sys_syslog(int type, char __user *buf, int len)
-{
-	return do_syslog(type, buf, len);
-}
-
 /*
  * Call the console drivers on a range of log_buf
  */

--

[patch 4/4] Distinguish /proc/kmsg access from sys_syslog

[Zack Weinberg]

[-- Attachment #1: distinguish_kmsg_security.diff --]
[-- Type: text/plain, Size: 3842 bytes --]

Finally, add a new security class for access to /proc/kmsg, distinct
from the class used for the "read current messages" operations on
sys_syslog.  The dummy and capability modules permit access to
/proc/kmsg to any user (who has somehow acquired an open fd on it);
SELinux is unchanged.  This accomplishes what I was trying to do in
the first place, i.e. enable running klogd unprivileged without a root
shim, in a non-SELinux installation.  (Please remember that the
default DAC permissions for /proc/kmsg restrict it to root, so unless
you chmod it in your installation or modify klogd to open the file and
then drop privs, the effective restrictions are unchanged.)

zw


Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-11-11 21:57:45.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-11-11 21:59:01.000000000 -0800
@@ -18,7 +18,7 @@
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	int error = security_syslog(KLOGSEC_READ);
+	int error = security_syslog(KLOGSEC_READ_PROC);
 	if (error)
 		return error;
 	return nonseekable_open(inode, file);
@@ -32,7 +32,7 @@
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	int error = security_syslog(KLOGSEC_READ);
+	int error = security_syslog(KLOGSEC_READ_PROC);
 	if (error)
 		return error;
 	return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
@@ -40,7 +40,7 @@
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-	int error = security_syslog(KLOGSEC_READ);
+	int error = security_syslog(KLOGSEC_READ_PROC);
 	if (error)
 		return error;
 	return klog_poll(file, wait);
Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-11-11 21:57:52.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-11-11 21:58:40.000000000 -0800
@@ -36,6 +36,8 @@
 #define KLOGSEC_READHIST  1  /* read message history (dmesg) */
 #define KLOGSEC_CLEARHIST 2  /* clear message history (dmesg -c) */
 #define KLOGSEC_CONSOLE   3  /* set console log level */
+#define KLOGSEC_READ_PROC 4  /* read current messages, but from /proc/kmsg
+				rather than the system call */
 
 /*
  * Subfunctions of sys_syslog used from fs/proc/kmsg.c.
Index: linux-2.6/security/commoncap.c
===================================================================
--- linux-2.6.orig/security/commoncap.c	2006-11-11 22:02:07.000000000 -0800
+++ linux-2.6/security/commoncap.c	2006-11-11 22:02:34.000000000 -0800
@@ -312,7 +312,14 @@
 
 int cap_syslog (int type)
 {
-	if (type != KLOGSEC_READHIST && !capable(CAP_SYS_ADMIN))
+	/*
+	 * Reading history is allowed to any user, and so is reading
+	 * current messages via /proc/kmsg (by default that file is
+	 * only readable by root, but root is allowed to change that,
+	 * or open it and hand the fd to an unprivileged process).
+	 */
+	if (type != KLOGSEC_READHIST && type != KLOGSEC_READ_PROC
+	    && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	return 0;
 }
Index: linux-2.6/security/selinux/hooks.c
===================================================================
--- linux-2.6.orig/security/selinux/hooks.c	2006-11-11 22:02:07.000000000 -0800
+++ linux-2.6/security/selinux/hooks.c	2006-11-11 22:04:11.000000000 -0800
@@ -1513,7 +1513,14 @@
 	case KLOGSEC_CONSOLE:
 		return task_has_system(current, SYSTEM__SYSLOG_CONSOLE);
 
+		/*
+		 * N.B. Unlike the default security model, with
+		 * SELinux active you have to have SYSTEM__SYSLOG_MOD
+		 * privilege to read current messages either with the
+		 * system call or from /proc/kmsg.
+		 */
 	case KLOGSEC_READ:
+	case KLOGSEC_READ_PROC:
 	case KLOGSEC_CLEARHIST:
 	default:
 		return task_has_system(current, SYSTEM__SYSLOG_MOD);

--

[patch 0/4] /proc/kmsg permissions, take three

[Zack Weinberg]

Here's a re-revised version of my patch set to allow klogd to drop
privileges and continue reading from /proc/kmsg (currently, even if klogd
has a legitimately opened fd on /proc/kmsg, it cannot read from it unless
it has CAP_SYS_ADMIN asserted).  SELinux's pickier and finer-grained
privilege rules for /proc/kmsg are unchanged.

The major change from the previous patchset
[q.v. http://comments.gmane.org/gmane.linux.kernel/466034 ] is that,
as Arjan van de Ven requested, the new header linux/klog.h contains only
userspace-visible definitions (the constants for sys_syslog()).  Thanks to
Alexey Dobriyan for telling me the proper place to put the KLOGSEC_*
constants (now renamed LSM_KLOG_* in keeping with other such constants).
They have also been rediffed versus yesterday's git.  They should be
applied in sequence; each step compiles, and the complete set has been
booted and tested to work as intended.

Any comments, as usual, appreciated.  I would very much like to see this
in 2.6.20.

zw

[patch 3/4] Refactor do_syslog interface

[Zack Weinberg]

[-- Attachment #1: break_up_do_syslog.diff --]
[-- Type: text/plain, Size: 10850 bytes --]

This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were with one exception: neither
kmsg_close() nor sys_syslog(KLOG_CLOSE, ...) calls security_syslog
at all anymore (close operations should never fail).

Finally, I fixed a couple of minor bugs.  __put_user error handling in
klog_read was slightly off: if __put_user returns an error, that
character should not be consumed from the kernel buffer; if it returns
an error after some characters have already been copied successfully,
the read operation should return the count of already-copied
characters, not the error code.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce that.

Change from previous version of patch: proc/kmsg.c declares the
kernel/printk.c interfaces itself, instead of getting them from klog.h
which people want to be purely userspace-visible constants.  kmsg.c has
always had private declarations of printk.c functions (before, there were
declarations of do_syslog and a wait queue there); as it is unlikely that
more users of these functions will appear, I think this will do fine.
(It might be reasonable to put declarations in console.h.)

zw

Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-12-13 16:04:46.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-12-13 16:36:56.000000000 -0800
@@ -12,40 +12,43 @@
 #include <linux/poll.h>
 #include <linux/fs.h>
 #include <linux/klog.h>
+#include <linux/security.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
-extern wait_queue_head_t log_wait;
-
-extern int do_syslog(int type, char __user *bug, int count);
+/* interfaces from kernel/printk.c */
+extern int klog_read(char __user *, int, int);
+extern unsigned int klog_poll(struct file *, poll_table *);
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(KLOG_OPEN,NULL,0);
+	int error = security_syslog(LSM_KLOG_READ);
+	if (error)
+		return error;
+	return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(KLOG_CLOSE,NULL,0);
 	return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	if ((file->f_flags & O_NONBLOCK)
-	    && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return -EAGAIN;
-	return do_syslog(KLOG_READ, buf, count);
+	int error = security_syslog(LSM_KLOG_READ);
+	if (error)
+		return error;
+	return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-	poll_wait(file, &log_wait, wait);
-	if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return POLLIN | POLLRDNORM;
-	return 0;
+	int error = security_syslog(LSM_KLOG_READ);
+	if (error)
+		return error;
+	return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-12-13 16:12:43.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-12-13 16:33:09.000000000 -0800
@@ -20,7 +20,9 @@
 				    * printed to console */
 
 	KLOG_GET_UNREAD      =  9, /* return number of unread characters */
-	KLOG_GET_SIZE        = 10  /* return size of log buffer */
+	KLOG_GET_SIZE        = 10, /* return size of log buffer */
+	KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+				    * -- new in 2.6.20 */
 };
 
 #endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-12-13 16:08:30.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-12-13 16:39:24.000000000 -0800
@@ -33,6 +33,7 @@
 #include <linux/syscalls.h>
 #include <linux/jiffies.h>
 #include <linux/klog.h>
+#include <linux/poll.h>
 
 #include <asm/uaccess.h>
 
@@ -45,7 +46,7 @@
 #define MINIMUM_CONSOLE_LOGLEVEL 1 /* Minimum loglevel we let people use */
 #define DEFAULT_CONSOLE_LOGLEVEL 7 /* anything MORE serious than KERN_DEBUG */
 
-DECLARE_WAIT_QUEUE_HEAD(log_wait);
+static DECLARE_WAIT_QUEUE_HEAD(log_wait);
 
 int console_printk[4] = {
 	DEFAULT_CONSOLE_LOGLEVEL,	/* console_loglevel */
@@ -164,116 +165,142 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-#define security_syslog_or_fail(type) do {		\
-		int error = security_syslog(type);	\
-		if (error)				\
-			return error;			\
-	} while (0)
+/*
+ * Subfunctions of sys_syslog.  Some are also used from fs/proc/kmsg.c.
+ */
 
-/* See linux/klog.h for the command numbers passed as the first argument.  */
-int do_syslog(int type, char __user *buf, int len)
+int klog_read(char __user *buf, int len, int block)
+{
+	int i, error;
+	char c;
+
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (len == 0)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+	if (!block && log_start - log_end == 0)
+		return -EAGAIN;
+
+	error = wait_event_interruptible(log_wait, (log_start - log_end));
+	if (error)
+		return error;
+
+	i = 0;
+	spin_lock_irq(&logbuf_lock);
+	while (log_start != log_end && i < len) {
+		c = LOG_BUF(log_start);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,buf);
+		if (error)
+			goto out;
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+		log_start++;
+		buf++;
+		i++;
+	}
+	spin_unlock_irq(&logbuf_lock);
+out:
+	if (i > 0)
+		return i;
+	return error;
+}
+
+unsigned int klog_poll(struct file *file, poll_table *wait)
+{
+	poll_wait(file, &log_wait, wait);
+	if (log_end - log_start > 0)
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
+static int klog_readhist(char __user *buf, int len)
 {
 	unsigned long i, j, limit, count;
-	int do_clear = 0;
 	char c;
 	int error = 0;
 
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (!len)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+
+	count = len;
+	if (count > log_buf_len)
+		count = log_buf_len;
+	spin_lock_irq(&logbuf_lock);
+	if (count > logged_chars)
+		count = logged_chars;
+	limit = log_end;
+	/*
+	 * __put_user() could sleep, and while we sleep
+	 * printk() could overwrite the messages
+	 * we try to copy to user space. Therefore
+	 * the messages are copied in reverse. <manfreds>
+	 */
+	for (i = 0; i < count && !error; i++) {
+		j = limit-1-i;
+		if (j + log_buf_len < log_end)
+			break;
+		c = LOG_BUF(j);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,&buf[count-1-i]);
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+	}
+	spin_unlock_irq(&logbuf_lock);
+	if (error)
+		return error;
+	error = i;
+	if (i != count) {
+		int offset = count-error;
+		/* buffer overflow during copy, correct user buffer. */
+		for (i = 0; i < error; i++) {
+			if (__get_user(c,&buf[i+offset]) ||
+			    __put_user(c,&buf[i])) {
+				error = -EFAULT;
+				break;
+			}
+			cond_resched();
+		}
+	}
+	return error;
+}
+
+#define security_syslog_or_fail(type) do {	\
+		error = security_syslog(type);	\
+		if (error)			\
+			return error;		\
+	} while (0)
+
+/* See linux/klog.h for the command numbers passed as the first argument.  */
+asmlinkage long sys_syslog(int type, char __user *buf, int len)
+{
+	int error = 0;
 	switch (type) {
 	case KLOG_CLOSE:
-		security_syslog_or_fail(LSM_KLOG_READ);
 		break;
 	case KLOG_OPEN:
 		security_syslog_or_fail(LSM_KLOG_READ);
 		break;
 	case KLOG_READ:
+	case KLOG_READ_NONBLOCK:
 		security_syslog_or_fail(LSM_KLOG_READ);
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		error = wait_event_interruptible(log_wait,
-							(log_start - log_end));
-		if (error)
-			goto out;
-		i = 0;
-		spin_lock_irq(&logbuf_lock);
-		while (!error && (log_start != log_end) && i < len) {
-			c = LOG_BUF(log_start);
-			log_start++;
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,buf);
-			buf++;
-			i++;
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (!error)
-			error = i;
+		error = klog_read(buf, len, type == KLOG_READ);
 		break;
 	case KLOG_READ_CLEAR_HIST:
+		security_syslog_or_fail(LSM_KLOG_READHIST);
 		security_syslog_or_fail(LSM_KLOG_CLEARHIST);
-		do_clear = 1;
-		/* FALL THRU */
+		error = klog_readhist(buf, len);
+		logged_chars = 0;
+		break;
 	case KLOG_READ_HIST:
 		security_syslog_or_fail(LSM_KLOG_READHIST);
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		count = len;
-		if (count > log_buf_len)
-			count = log_buf_len;
-		spin_lock_irq(&logbuf_lock);
-		if (count > logged_chars)
-			count = logged_chars;
-		if (do_clear)
-			logged_chars = 0;
-		limit = log_end;
-		/*
-		 * __put_user() could sleep, and while we sleep
-		 * printk() could overwrite the messages
-		 * we try to copy to user space. Therefore
-		 * the messages are copied in reverse. <manfreds>
-		 */
-		for (i = 0; i < count && !error; i++) {
-			j = limit-1-i;
-			if (j + log_buf_len < log_end)
-				break;
-			c = LOG_BUF(j);
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,&buf[count-1-i]);
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (error)
-			break;
-		error = i;
-		if (i != count) {
-			int offset = count-error;
-			/* buffer overflow during copy, correct user buffer. */
-			for (i = 0; i < error; i++) {
-				if (__get_user(c,&buf[i+offset]) ||
-				    __put_user(c,&buf[i])) {
-					error = -EFAULT;
-					break;
-				}
-				cond_resched();
-			}
-		}
+		error = klog_readhist(buf, len);
 		break;
 	case KLOG_CLEAR_HIST:
 		security_syslog_or_fail(LSM_KLOG_CLEARHIST);
@@ -291,7 +318,7 @@
 		security_syslog_or_fail(LSM_KLOG_CONSOLE);
 		error = -EINVAL;
 		if (len < 1 || len > 8)
-			goto out;
+			break;
 		if (len < minimum_console_loglevel)
 			len = minimum_console_loglevel;
 		console_loglevel = len;
@@ -316,15 +343,9 @@
 		error = -EINVAL;
 		break;
 	}
-out:
 	return error;
 }
 
-asmlinkage long sys_syslog(int type, char __user *buf, int len)
-{
-	return do_syslog(type, buf, len);
-}
-
 /*
  * Call the console drivers on a range of log_buf
  */

--

[patch 0/4] /proc/kmsg permissions, take four

[Zack Weinberg]

Here's yet another revision of the /proc/kmsg permissions patch
series.  To recap, the point is to allow klogd to drop privileges
and continue reading from /proc/kmsg (currently, even if klogd has a
legitimately opened fd on /proc/kmsg, it cannot read from it unless
it has CAP_SYS_ADMIN asserted).  SELinux's pickier and finer-grained
privilege rules for /proc/kmsg are unchanged.

There are two significant changes from the previous revision.  First,
in keeping with the recommended style, I have eliminated the
security_syslog_or_fail() macro.  Instead there is a static array mapping
KLOG_* opcodes to LSM_KLOG_* privilege classes.  This requires slightly
different coding in the security hooks but I think it's clearer overall.
Second, I've incorporated Vincent Legoll's kerneldoc comment for sys_syslog
(nee do_syslog) with some wording improvements and expansion to cover the
klog_* functions introduced part-way through the patch.  I don't think
proc/kmsg.c needs kerneldoc, it's very simple after this patch series.

I've been through Documentation/CodingStyle and satisfied myself that
everything is now in the proper mode.  I don't suppose anyone has comments
on the *content* of the changes...?

zw

[patch 3/4] Refactor do_syslog interface

[Zack Weinberg]

[-- Attachment #1: break_up_do_syslog.diff --]
[-- Type: text/plain, Size: 12951 bytes --]

This patch breaks out the read operations in do_syslog() into their
own functions (klog_read, klog_readhist) and adds a klog_poll.
klog_read grows the ability to do a nonblocking read, which I expose
in the sys_syslog interface because there doesn't seem to be any
reason not to.  do_syslog itself is folded into sys_syslog.  The
security checks remain there, not in the subfunctions.  I've added
kerneldoc comments for all the new functions in printk.c.  The control
flow in sys_syslog is different in style but not in substance -- I
like direct returns rather than setting a local variable and returning
at the end; it lets me put a BUG() after the switch statement to catch
missed cases.  [gcc4 at least can optimize it out when there aren't
any.]

kmsg.c is then changed to use those functions instead of calling
do_syslog and/or poll_wait itself.. This entails that it must call
security_syslog as appropriate itself.  In this patch I preserve the
security checks exactly as they were after the changes in the "map
permissions" patch (i.e. kmsg_close() doesn't do a permission check).

Finally, I fixed some minor bugs. Error and partial read handling in
klog_read/klog_readhist were slightly off: if __put_user returns an
error, that character should not be consumed from the kernel buffer;
if it returns an error after some characters have already been copied
successfully, the read operation should return the count of
already-copied characters, not the error code; if less than the entire
buffer is read with KLOG_READ_CLEAR_HIST, we should only clear the
part that was successfully read.  Seeking on /proc/kmsg has never been
meaningful, so kmsg_open() should call nonseekable_open() to enforce
that.

proc/kmsg.c declares the kernel/printk.c interfaces itself, instead of
getting them from klog.h which people want to be purely userspace-
visible constants.  kmsg.c has always had private declarations of
printk.c functions (before, there were declarations of do_syslog and a
wait queue there); as it is unlikely that more users of these
functions will appear, I think this will do fine.  (It might be
reasonable to put declarations in console.h.)

It was possible to prune down the set of headers included by kmsg.c
quite substantially.

zw

Index: linux-2.6/fs/proc/kmsg.c
===================================================================
--- linux-2.6.orig/fs/proc/kmsg.c	2006-12-24 11:43:14.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-12-24 11:43:18.000000000 -0800
@@ -5,47 +5,42 @@
  *
  */
 
-#include <linux/types.h>
-#include <linux/errno.h>
-#include <linux/time.h>
-#include <linux/kernel.h>
-#include <linux/poll.h>
 #include <linux/fs.h>
-#include <linux/klog.h>
-
-#include <asm/uaccess.h>
-#include <asm/io.h>
-
-extern wait_queue_head_t log_wait;
+#include <linux/poll.h>
+#include <linux/security.h>
 
-extern int do_syslog(int type, char __user *bug, int count);
+/* interfaces from kernel/printk.c */
+extern int klog_read(char __user *, int, int);
+extern unsigned int klog_poll(struct file *, poll_table *);
 
 static int kmsg_open(struct inode * inode, struct file * file)
 {
-	return do_syslog(KLOG_OPEN, NULL, 0);
+	int error = security_syslog(LSM_KLOG_READ);
+	if (error)
+		return error;
+	return nonseekable_open(inode, file);
 }
 
 static int kmsg_release(struct inode * inode, struct file * file)
 {
-	(void) do_syslog(KLOG_CLOSE, NULL, 0);
 	return 0;
 }
 
 static ssize_t kmsg_read(struct file *file, char __user *buf,
 			 size_t count, loff_t *ppos)
 {
-	if ((file->f_flags & O_NONBLOCK)
-	    && !do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return -EAGAIN;
-	return do_syslog(KLOG_READ, buf, count);
+	int error = security_syslog(LSM_KLOG_READ);
+	if (error)
+		return error;
+	return klog_read(buf, count, !(file->f_flags & O_NONBLOCK));
 }
 
 static unsigned int kmsg_poll(struct file *file, poll_table *wait)
 {
-	poll_wait(file, &log_wait, wait);
-	if (do_syslog(KLOG_GET_UNREAD, NULL, 0))
-		return POLLIN | POLLRDNORM;
-	return 0;
+	int error = security_syslog(LSM_KLOG_READ);
+	if (error)
+		return error;
+	return klog_poll(file, wait);
 }
 
 
Index: linux-2.6/include/linux/klog.h
===================================================================
--- linux-2.6.orig/include/linux/klog.h	2006-12-24 11:43:14.000000000 -0800
+++ linux-2.6/include/linux/klog.h	2006-12-24 11:43:18.000000000 -0800
@@ -21,6 +21,8 @@
 
 	KLOG_GET_UNREAD      =  9, /* return number of unread characters */
 	KLOG_GET_SIZE        = 10, /* return size of log buffer */
+ 	KLOG_READ_NONBLOCK   = 11, /* read from log, don't block if empty
+ 				    * -- new in 2.6.20 */
 
 	KLOG_OPCODE_MAX
 };
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-12-24 11:43:16.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-12-24 11:43:18.000000000 -0800
@@ -33,6 +33,7 @@
 #include <linux/syscalls.h>
 #include <linux/jiffies.h>
 #include <linux/klog.h>
+#include <linux/poll.h>
 
 #include <asm/uaccess.h>
 
@@ -45,7 +46,7 @@
 #define MINIMUM_CONSOLE_LOGLEVEL 1 /* Minimum loglevel we let people use */
 #define DEFAULT_CONSOLE_LOGLEVEL 7 /* anything MORE serious than KERN_DEBUG */
 
-DECLARE_WAIT_QUEUE_HEAD(log_wait);
+static DECLARE_WAIT_QUEUE_HEAD(log_wait);
 
 int console_printk[4] = {
 	DEFAULT_CONSOLE_LOGLEVEL,	/* console_loglevel */
@@ -165,6 +166,136 @@
 __setup("log_buf_len=", log_buf_len_setup);
 
 /*
+ * Subfunctions of sys_syslog.  Some are also used from fs/proc/kmsg.c.
+ */
+
+/**
+ * klog_read - read current messages from the kernel log buffer.
+ * @buf: user-space buffer to copy data into
+ * @len: number of bytes of space available at @buf
+ * @block: if nonzero, block until data is available.
+ *
+ * Return value is as for the read() system call.
+ */
+int klog_read(char __user *buf, int len, int block)
+{
+	int i, error;
+	char c;
+
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (len == 0)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+	if (!block && log_start - log_end == 0)
+		return -EAGAIN;
+
+	error = wait_event_interruptible(log_wait, (log_start - log_end));
+	if (error)
+		return error;
+
+	i = 0;
+	spin_lock_irq(&logbuf_lock);
+	while (log_start != log_end && i < len) {
+		c = LOG_BUF(log_start);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,buf);
+		if (error)
+			goto out;
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+		log_start++;
+		buf++;
+		i++;
+	}
+	spin_unlock_irq(&logbuf_lock);
+out:
+	if (i > 0)
+		return i;
+	return error;
+}
+
+/**
+ * klog_poll - poll() worker for the kernel log buffer.
+ */
+unsigned int klog_poll(struct file *file, poll_table *wait)
+{
+	poll_wait(file, &log_wait, wait);
+	if (log_end - log_start > 0)
+		return POLLIN | POLLRDNORM;
+	return 0;
+}
+
+/**
+ * klog_read - read historic messages from the kernel log buffer.
+ * @buf: user-space buffer to copy data into
+ * @len: number of bytes of space available at @buf
+ * @block: if nonzero, block until data is available.
+ *
+ * Return value is as for the read() system call.  The difference
+ * between this and klog_read() is that this function will copy out all
+ * the messages that have ever been reported by the kernel, as long as
+ * they've not been overwritten by newer messages; whereas klog_read()
+ * only ever copies out a given message once.
+ */
+static int klog_readhist(char __user *buf, int len)
+{
+	unsigned long i, j, limit, count;
+	char c;
+	int error = 0;
+
+	if (!buf || len < 0)
+		return -EINVAL;
+	if (!len)
+		return 0;
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+
+	count = len;
+	if (count > log_buf_len)
+		count = log_buf_len;
+	spin_lock_irq(&logbuf_lock);
+	if (count > logged_chars)
+		count = logged_chars;
+	limit = log_end;
+	/*
+	 * __put_user() could sleep, and while we sleep
+	 * printk() could overwrite the messages
+	 * we try to copy to user space. Therefore
+	 * the messages are copied in reverse. <manfreds>
+	 */
+	for (i = 0; i < count && !error; i++) {
+		j = limit-1-i;
+		if (j + log_buf_len < log_end)
+			break;
+		c = LOG_BUF(j);
+		spin_unlock_irq(&logbuf_lock);
+		error = __put_user(c,&buf[count-1-i]);
+		cond_resched();
+		spin_lock_irq(&logbuf_lock);
+	}
+	spin_unlock_irq(&logbuf_lock);
+	if (error)
+		return error;
+	error = i;
+	if (i != count) {
+		int offset = count-error;
+		/* buffer overflow during copy, correct user buffer. */
+		for (i = 0; i < error; i++) {
+			if (__get_user(c,&buf[i+offset]) ||
+			    __put_user(c,&buf[i])) {
+				error = -EFAULT;
+				break;
+			}
+			cond_resched();
+		}
+	}
+	return error;
+}
+
+
+/*
  * This table maps KLOG_* operation codes to LSM_KLOG_* privilege classes.
  * "unsigned char" is used just to keep it small.
  */
@@ -174,6 +305,7 @@
 	[KLOG_OPEN]            = LSM_KLOG_READ,
 	[KLOG_READ]            = LSM_KLOG_READ,
 	[KLOG_GET_UNREAD]      = LSM_KLOG_READ,
+	[KLOG_READ_NONBLOCK]   = LSM_KLOG_READ,
 
 	[KLOG_READ_HIST]       = LSM_KLOG_READHIST,
 	[KLOG_GET_SIZE]        = LSM_KLOG_READHIST,
@@ -198,7 +330,7 @@
 ];
 
 /**
- * do_syslog - operate on the log of kernel messages
+ * sys_syslog - operate on the log of kernel messages
  * @type: operation to perform
  * @buf: user-space buffer to copy data into
  * @len: number of bytes of space available at @buf
@@ -213,11 +345,8 @@
  * On failure, returns a negative errno code.  On success, returns a
  * nonnegative integer whose meaning depends on @type.
  */
-int do_syslog(int type, char __user *buf, int len)
+asmlinkage long sys_syslog(int type, char __user *buf, int len)
 {
-	unsigned long i, j, limit, count;
-	int do_clear = 0;
-	char c;
 	int error;
 
 	if (type < 0 || type >= KLOG_OPCODE_MAX)
@@ -228,131 +357,51 @@
 
 	switch (type) {
 	case KLOG_CLOSE:
-		break;
 	case KLOG_OPEN:
-		break;
+		return 0;
+
 	case KLOG_READ:
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		error = wait_event_interruptible(log_wait,
-							(log_start - log_end));
-		if (error)
-			goto out;
-		i = 0;
-		spin_lock_irq(&logbuf_lock);
-		while (!error && (log_start != log_end) && i < len) {
-			c = LOG_BUF(log_start);
-			log_start++;
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,buf);
-			buf++;
-			i++;
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (!error)
-			error = i;
-		break;
+	case KLOG_READ_NONBLOCK:
+		return klog_read(buf, len, type == KLOG_READ);
+
 	case KLOG_READ_CLEAR_HIST:
-		do_clear = 1;
-		/* FALL THRU */
+		error = klog_readhist(buf, len);
+		if (error > 0)
+			logged_chars -= error;
+		return error;
+
 	case KLOG_READ_HIST:
-		error = -EINVAL;
-		if (!buf || len < 0)
-			goto out;
-		error = 0;
-		if (!len)
-			goto out;
-		if (!access_ok(VERIFY_WRITE, buf, len)) {
-			error = -EFAULT;
-			goto out;
-		}
-		count = len;
-		if (count > log_buf_len)
-			count = log_buf_len;
-		spin_lock_irq(&logbuf_lock);
-		if (count > logged_chars)
-			count = logged_chars;
-		if (do_clear)
-			logged_chars = 0;
-		limit = log_end;
-		/*
-		 * __put_user() could sleep, and while we sleep
-		 * printk() could overwrite the messages
-		 * we try to copy to user space. Therefore
-		 * the messages are copied in reverse. <manfreds>
-		 */
-		for (i = 0; i < count && !error; i++) {
-			j = limit-1-i;
-			if (j + log_buf_len < log_end)
-				break;
-			c = LOG_BUF(j);
-			spin_unlock_irq(&logbuf_lock);
-			error = __put_user(c,&buf[count-1-i]);
-			cond_resched();
-			spin_lock_irq(&logbuf_lock);
-		}
-		spin_unlock_irq(&logbuf_lock);
-		if (error)
-			break;
-		error = i;
-		if (i != count) {
-			int offset = count-error;
-			/* buffer overflow during copy, correct user buffer. */
-			for (i = 0; i < error; i++) {
-				if (__get_user(c,&buf[i+offset]) ||
-				    __put_user(c,&buf[i])) {
-					error = -EFAULT;
-					break;
-				}
-				cond_resched();
-			}
-		}
-		break;
+		return klog_readhist(buf, len);
+
 	case KLOG_CLEAR_HIST:
 		logged_chars = 0;
-		break;
+		return 0;
+
 	case KLOG_DISABLE_CONSOLE:
 		console_loglevel = minimum_console_loglevel;
-		break;
+		return 0;
+
 	case KLOG_ENABLE_CONSOLE:
 		console_loglevel = default_console_loglevel;
-		break;
+		return 0;
+
 	case KLOG_SET_CONSOLE_LVL:
-		error = -EINVAL;
 		if (len < 1 || len > 8)
-			goto out;
+			return -EINVAL;
+
 		if (len < minimum_console_loglevel)
 			len = minimum_console_loglevel;
 		console_loglevel = len;
-		error = 0;
-		break;
+		return 0;
+
 	case KLOG_GET_UNREAD:
-		error = log_end - log_start;
-		break;
+		return log_end - log_start;
+
 	case KLOG_GET_SIZE:
-		error = log_buf_len;
-		break;
-	default:
-		error = -EINVAL;
-		break;
+		return log_buf_len;
 	}
-out:
-	return error;
-}
 
-asmlinkage long sys_syslog(int type, char __user *buf, int len)
-{
-	return do_syslog(type, buf, len);
+	BUG();
 }
 
 /*

--