Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n

[Linus Torvalds]

On Sat, Nov 13, 2010 at 9:26 AM, Joe Perches <joe@xxxxxxxxxxx> wrote:
>
> dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
> Its uses need to be guarded as well.

Fair enough, but I think this part:

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 04b80f9..29f2368 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
>  {
>        if (type != SYSLOG_ACTION_OPEN && from_file)
>                return 0;
> +#ifdef CONFIG_PRINTK
>        if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
>                return -EPERM;
> +#endif
>        if ((type != SYSLOG_ACTION_READ_ALL &&
>             type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
>                return -EPERM;

is incredibly ugly. If CONFIG_PRINTK isn't set, the whole function
just becomes pointless, so why guard just that one part of it?

So I would suggest guarding the whole thing, and just returning 0 if
CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
test into do_syslog, and stop playing stupid games with
"security_syslog()", which actually goes away if you disable the you
disable CONFIG_SECURITY.

SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
doing it in security_syslog() was a bug to begin with.

Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
and move it entirely into security/commoncap.c, and not pollute
kernel/printk.c at all with it.

Anyway, suggested replacement patch attached. Comments?

                         Linus
 kernel/printk.c      |    3 +++
 kernel/sysctl.c      |    2 +-
 security/commoncap.c |    2 --
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 38e7d58..15dd585 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -274,6 +274,9 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	char c;
 	int error = 0;
 
+	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	error = security_syslog(type, from_file);
 	if (error)
 		return error;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5abfa15 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -702,7 +702,6 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &ten_thousand,
 	},
-#endif
 	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
@@ -712,6 +711,7 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..5e632b4 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -895,8 +895,6 @@ int cap_syslog(int type, bool from_file)
 {
 	if (type != SYSLOG_ACTION_OPEN && from_file)
 		return 0;
-	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
-		return -EPERM;
 	if ((type != SYSLOG_ACTION_READ_ALL &&
 	     type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
 		return -EPERM;