Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
> Fine Fine, I'll get off my lazy butt and look at this.

Shock!

> Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
> and the syscall both use do_syslog() which calls
> check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
> security_syslog(), which we all agree needs fixed.
> 
> > > Also, the LSM hooks aren't doing any capability checks at all that I can
> > > see, which may or may not be a bug in and of itself but I have no idea.
> > > I was hoping Eric would speak up about that.
> 
> I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
> to have security_* sometimes the right thing to do and sometimes
> capable() is the right thing to do.  It is pervasive in the kernel that
> you have either/or, but I can't think of anywhere that functions are
> expected to do BOTH.  So yeah, that needs fixed.

OK.

> 
> > Eric explicitly removed the cap check since it was cluttering things
> > the way it was originally written. I do think security_syslog() should
> > pass through check_syslog_permissions(), though. Then this wouldn't
> > have happened. That might actually be the right way to clean this up,
> > but I'd like to see Eric's thoughts first.
> 
> How about something like this?

I think this looks pretty good.  Much clearer overall and the
consolidation is nice.  I'll try to get it tested soon.

josh

> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 7c69b3e..ced2cac 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
>  	if (err)
>  		return err;
>  
> @@ -840,22 +840,23 @@ static int check_syslog_permissions(int type, bool from_file)
>  	 * already done the capabilities checks at open time.
>  	 */
>  	if (from_file && type != SYSLOG_ACTION_OPEN)
> -		return 0;
> +		goto ok;
>  
>  	if (syslog_action_restricted(type)) {
>  		if (capable(CAP_SYSLOG))
> -			return 0;
> +			goto ok;
>  		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>  		if (capable(CAP_SYS_ADMIN)) {
>  			printk_once(KERN_WARNING "%s (%d): "
>  				 "Attempt to access syslog with CAP_SYS_ADMIN "
>  				 "but no CAP_SYSLOG (deprecated).\n",
>  				 current->comm, task_pid_nr(current));
> -			return 0;
> +			goto ok;
>  		}
>  		return -EPERM;
>  	}
> -	return 0;
> +ok:
> +	return security_syslog(type);
>  }
>  
>  #if defined(CONFIG_PRINTK_TIME)
> @@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	if (error)
>  		goto out;
>  
> -	error = security_syslog(type);
> -	if (error)
> -		return error;
> -
>  	switch (type) {
>  	case SYSLOG_ACTION_CLOSE:	/* Close log */
>  		break;
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/