[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 1/4] Add <linux/klog.h>

[Zack Weinberg]

[-- Attachment #1: add_klog_h.diff --]
[-- Type: text/plain, Size: 5835 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.  (Unlike the previous revision of this
patch series, no kernel-private additions to this file are contemplated.)

zw

Index: linux-2.6/include/linux/Kbuild
===================================================================
--- linux-2.6.orig/include/linux/Kbuild	2006-12-13 15:58:13.000000000 -0800
+++ linux-2.6/include/linux/Kbuild	2006-12-13 16:06:57.000000000 -0800
@@ -100,6 +100,7 @@
 header-y += ixjuser.h
 header-y += jffs2.h
 header-y += keyctl.h
+header-y += klog.h
 header-y += limits.h
 header-y += lock_dlm_plock.h
 header-y += magic.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-12-13 16:06:22.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-12-13 15:58:16.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-12-13 16:06:22.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>
 
@@ -163,21 +164,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;
@@ -190,11 +177,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;
@@ -225,10 +212,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;
@@ -281,16 +268,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;
@@ -299,10 +286,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-12-13 15:53:29.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-12-13 16:04:46.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;
 }

--

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

[Randy Dunlap]

On Thu, 14 Dec 2006 16:16:40 -0800 Zack Weinberg wrote:

> 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.  (Unlike the previous revision of this
> patch series, no kernel-private additions to this file are contemplated.)

Hi Zack,

This patch looks good except for one nit:

> --- linux-2.6.orig/fs/proc/kmsg.c	2006-12-13 15:53:29.000000000 -0800
> +++ linux-2.6/fs/proc/kmsg.c	2006-12-13 16:04:46.000000000 -0800
> @@ -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;
>  }

Please use a space after the commas (even though you just left it
as it already was).

---
~Randy

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

[Zack Weinberg]

On 12/14/06, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > -     (void) do_syslog(0,NULL,0);
> > +     (void) do_syslog(KLOG_CLOSE,NULL,0);
>
> Please use a space after the commas (even though you just left it
> as it already was).

Will change for the next revision.

zw

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

[Vincent Legoll]

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]

Hello,

what about something along the lines of the following,
on top of your patch ?

Or should the kernel-doc be put on another function
instead of that one ?

-- 
Vincent Legoll

[-- Attachment #2: do-syslog-kernel-doc --]
[-- Type: text/plain, Size: 1178 bytes --]

Add do_syslog() kernel-doc

---
commit 95b0721d8b4b46ddf83113fe49492810d7d92060
tree e2715a8cf7eb0d71b3bee2185a5cf98639d79d90
parent de794d2dfd6dd0c38dd552020ac00c46e1df5293
author Vincent Legoll <vincent.legoll@gmail.com> Wed, 20 Dec 2006 01:29:34 +0100
committer Vincent Legoll <vincent.legoll@gmail.com> Wed, 20 Dec 2006 01:29:34 +0100

 kernel/printk.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 232467e..5416d07 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -164,7 +164,16 @@ out:
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-/* See linux/klog.h for the command numbers passed as the first argument.  */
+/**
+ * do_syslog - operate on kernel messages log
+ * @type: operation to perform
+ * @buf: user-space buffer to copy data into
+ * @len: length of data to copy from log into @buf
+ *
+ * See include/linux/klog.h for the command numbers passed as @type.
+ * Parameters @buf & @len are only used for operations of type %KLOG_READ,
+ * %KLOG_READ_HIST and %KLOG_READ_CLEAR_HIST.
+ */
 int do_syslog(int type, char __user *buf, int len)
 {
 	unsigned long i, j, limit, count;

[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 1/4] Add <linux/klog.h>

[Zack Weinberg]

[-- Attachment #1: add_klog_h.diff --]
[-- Type: text/plain, Size: 6322 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.

zw

Index: linux-2.6/include/linux/Kbuild
===================================================================
--- linux-2.6.orig/include/linux/Kbuild	2006-12-23 08:56:15.000000000 -0800
+++ linux-2.6/include/linux/Kbuild	2006-12-24 11:43:14.000000000 -0800
@@ -100,6 +100,7 @@
 header-y += ixjuser.h
 header-y += jffs2.h
 header-y += keyctl.h
+header-y += klog.h
 header-y += limits.h
 header-y += lock_dlm_plock.h
 header-y += magic.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-12-24 11:43:14.000000000 -0800
@@ -0,0 +1,28 @@
+#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 */
+
+	KLOG_OPCODE_MAX
+};
+
+#endif /* klog.h */
Index: linux-2.6/kernel/printk.c
===================================================================
--- linux-2.6.orig/kernel/printk.c	2006-12-23 08:56:15.000000000 -0800
+++ linux-2.6/kernel/printk.c	2006-12-24 11:43:14.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>
 
@@ -163,20 +164,21 @@
 
 __setup("log_buf_len=", log_buf_len_setup);
 
-/*
- * Commands to do_syslog:
+/**
+ * do_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
+ *
+ * See include/linux/klog.h for the command numbers passed as @type.
+ * The @buf and @len parameters are used with the above meanings for
+ * @type values %KLOG_READ, %KLOG_READ_HIST and %KLOG_READ_CLEAR_HIST.
+ * @len is reused with a different meaning, and @buf ignored, for
+ * %KLOG_SET_CONSOLE_LVL.  Both @buf and @len are ignored for all
+ * other @type values.
  *
- * 	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
+ * 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)
 {
@@ -190,11 +192,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;
@@ -225,10 +227,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;
@@ -281,16 +283,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;
@@ -299,10 +301,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-12-23 08:55:17.000000000 -0800
+++ linux-2.6/fs/proc/kmsg.c	2006-12-24 11:43:14.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;
 }

--

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

[Jan Engelhardt]

On Dec 24 2006 12:22, Zack Weinberg wrote:
>===================================================================
>--- linux-2.6.orig/fs/proc/kmsg.c	2006-12-23 08:55:17.000000000 -0800
>+++ linux-2.6/fs/proc/kmsg.c	2006-12-24 11:43:14.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);
        ^^^^^^

I bet you can get rid of that in the process.


	-`J'
-- 

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

[Vincent Legoll]

Zack Weinberg wrote:
> 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.

[...]

> -/*
> - * Commands to do_syslog:
> +/**
> + * do_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
> + *
> + * See include/linux/klog.h for the command numbers passed as @type.
> + * The @buf and @len parameters are used with the above meanings for
> + * @type values %KLOG_READ, %KLOG_READ_HIST and %KLOG_READ_CLEAR_HIST.
> + * @len is reused with a different meaning, and @buf ignored, for
> + * %KLOG_SET_CONSOLE_LVL.  Both @buf and @len are ignored for all
> + * other @type values.
>   *
> - * 	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
> + * 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)
>  {
> @@ -190,11 +192,11 @@

That part looks good to me, and the kernel-doc hunks from
"[patch 3/4] Refactor do_syslog interface"
too, are:

Acked-by: Vincent Legoll <vincent.legoll@gmail.com>

thanks for handling that.

-- 
Vincent Legoll