linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3)
@ 2011-02-10 14:40 Serge E. Hallyn
  2011-02-10 19:16 ` Linus Torvalds
  0 siblings, 1 reply; 11+ messages in thread
From: Serge E. Hallyn @ 2011-02-10 14:40 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Gergely Nagy, david, Alan Cox, Marc Koschewski, lkml,
	James Morris, Nick Bowler

In commit ce6ada35bdf710d16582cc4869c26722547e6f11, Serge messed up
userspace backward compatibility.  As Gergely pointed out, userspace
which was doing the right thing and dropping all but cap_sys_admin
before calling syslog is now breaking.

At 2.6.39 or 2.6.40, let's add a sysctl which defaults to 1.  When
0, if the user has cap_sys_admin but no cap_syslog, return -EPERM.
When 1 in that case, allow.  Alternatively, as David pointed out,
just leaving the behavior as below is still very useful.

Please apply.

Signed-off-by: Serge Hallyn <serge@hallyn.com>
---
 kernel/printk.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 2ddbdc7..bc56386 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -274,12 +274,24 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	 * at open time.
 	 */
 	if (type == SYSLOG_ACTION_OPEN || !from_file) {
-		if (dmesg_restrict && !capable(CAP_SYSLOG))
-			goto warn; /* switch to return -EPERM after 2.6.39 */
+		if (dmesg_restrict && !capable(CAP_SYSLOG)) {
+			/* remove after 2.6.39 */
+			if (capable(CAP_SYS_ADMIN))
+				WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
+				  "but no CAP_SYSLOG (deprecated).\n");
+			else
+				return -EPERM;
+		}
 		if ((type != SYSLOG_ACTION_READ_ALL &&
 		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
-		    !capable(CAP_SYSLOG))
-			goto warn; /* switch to return -EPERM after 2.6.39 */
+		     !capable(CAP_SYSLOG)) {
+			/* remove after 2.6.39 */
+			if (capable(CAP_SYS_ADMIN))
+				WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
+				  "but no CAP_SYSLOG (deprecated).\n");
+			else
+				return -EPERM;
+		}
 	}
 
 	error = security_syslog(type);
@@ -423,12 +435,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	}
 out:
 	return error;
-warn:
-	/* remove after 2.6.39 */
-	if (capable(CAP_SYS_ADMIN))
-		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
-		  "but no CAP_SYSLOG (deprecated and denied).\n");
-	return -EPERM;
 }
 
 SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
-- 
1.7.2.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3)
  2011-02-10 14:40 [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3) Serge E. Hallyn
@ 2011-02-10 19:16 ` Linus Torvalds
  2011-02-10 22:43   ` Serge E. Hallyn
                     ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Linus Torvalds @ 2011-02-10 19:16 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Gergely Nagy, david, Alan Cox, Marc Koschewski, lkml,
	James Morris, Nick Bowler

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

On Thu, Feb 10, 2011 at 6:40 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
>
> Please apply.

Hmm. So I detest the duplication and the ugly resulting code.  It was
a bit hard to follow before, now it's just nasty.

Why not make this all into some nicer helper functions instead -
something simple like the attached?

UNTESTED! I'm not going to apply it unless I get acks/tested-by's.

                                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2304 bytes --]

 kernel/printk.c |   54 +++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 2ddbdc7..f51214c 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -262,25 +262,47 @@ int dmesg_restrict = 1;
 int dmesg_restrict;
 #endif
 
+static int syslog_action_restricted(int type)
+{
+	if (dmesg_restrict)
+		return 1;
+	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
+	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
+}
+
+static int check_syslog_permissions(int type, bool from_file)
+{
+	/*
+	 * If this is from /proc/kmsg and we've already opened it, then we've
+	 * already the capabilities checks at open time.
+	 */
+	if (from_file && type != SYSLOG_ACTION_OPEN)
+		return 0;
+
+	if (syslog_action_restricted(type)) {
+		if (capable(CAP_SYSLOG))
+			return 0;
+		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
+		if (capable(CAP_SYS_ADMIN)) {
+			WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
+				 "but no CAP_SYSLOG (deprecated).\n");
+			return 0;
+		}
+		return -EPERM;
+	}
+	return 0;
+}
+
 int do_syslog(int type, char __user *buf, int len, bool from_file)
 {
 	unsigned i, j, limit, count;
 	int do_clear = 0;
 	char c;
-	int error = 0;
+	int error;
 
-	/*
-	 * If this is from /proc/kmsg we only do the capabilities checks
-	 * at open time.
-	 */
-	if (type == SYSLOG_ACTION_OPEN || !from_file) {
-		if (dmesg_restrict && !capable(CAP_SYSLOG))
-			goto warn; /* switch to return -EPERM after 2.6.39 */
-		if ((type != SYSLOG_ACTION_READ_ALL &&
-		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
-		    !capable(CAP_SYSLOG))
-			goto warn; /* switch to return -EPERM after 2.6.39 */
-	}
+	error = check_syslog_permissions(type, from_file);
+	if (error)
+		goto out;
 
 	error = security_syslog(type);
 	if (error)
@@ -423,12 +445,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	}
 out:
 	return error;
-warn:
-	/* remove after 2.6.39 */
-	if (capable(CAP_SYS_ADMIN))
-		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
-		  "but no CAP_SYSLOG (deprecated and denied).\n");
-	return -EPERM;
 }
 
 SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3)
  2011-02-10 19:16 ` Linus Torvalds
@ 2011-02-10 22:43   ` Serge E. Hallyn
  2011-02-10 22:59   ` James Morris
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Serge E. Hallyn @ 2011-02-10 22:43 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Serge E. Hallyn, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, James Morris, Nick Bowler

Quoting Linus Torvalds (torvalds@linux-foundation.org):
> On Thu, Feb 10, 2011 at 6:40 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > Please apply.
> 
> Hmm. So I detest the duplication and the ugly resulting code.  It was
> a bit hard to follow before, now it's just nasty.
> 
> Why not make this all into some nicer helper functions instead -
> something simple like the attached?
> 
> UNTESTED! I'm not going to apply it unless I get acks/tested-by's.
> 
>                                   Linus

It looks correct (and much nicer indeed)

Acked-by: Serge Hallyn <serge@hallyn.com>

Setting up a box to actually test on...

thanks,
-serge

>  kernel/printk.c |   54 +++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 2ddbdc7..f51214c 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -262,25 +262,47 @@ int dmesg_restrict = 1;
>  int dmesg_restrict;
>  #endif
>  
> +static int syslog_action_restricted(int type)
> +{
> +	if (dmesg_restrict)
> +		return 1;
> +	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> +	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +	/*
> +	 * If this is from /proc/kmsg and we've already opened it, then we've
> +	 * already the capabilities checks at open time.
> +	 */
> +	if (from_file && type != SYSLOG_ACTION_OPEN)
> +		return 0;
> +
> +	if (syslog_action_restricted(type)) {
> +		if (capable(CAP_SYSLOG))
> +			return 0;
> +		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> +		if (capable(CAP_SYS_ADMIN)) {
> +			WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> +				 "but no CAP_SYSLOG (deprecated).\n");
> +			return 0;
> +		}
> +		return -EPERM;
> +	}
> +	return 0;
> +}
> +
>  int do_syslog(int type, char __user *buf, int len, bool from_file)
>  {
>  	unsigned i, j, limit, count;
>  	int do_clear = 0;
>  	char c;
> -	int error = 0;
> +	int error;
>  
> -	/*
> -	 * If this is from /proc/kmsg we only do the capabilities checks
> -	 * at open time.
> -	 */
> -	if (type == SYSLOG_ACTION_OPEN || !from_file) {
> -		if (dmesg_restrict && !capable(CAP_SYSLOG))
> -			goto warn; /* switch to return -EPERM after 2.6.39 */
> -		if ((type != SYSLOG_ACTION_READ_ALL &&
> -		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
> -		    !capable(CAP_SYSLOG))
> -			goto warn; /* switch to return -EPERM after 2.6.39 */
> -	}
> +	error = check_syslog_permissions(type, from_file);
> +	if (error)
> +		goto out;
>  
>  	error = security_syslog(type);
>  	if (error)
> @@ -423,12 +445,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	}
>  out:
>  	return error;
> -warn:
> -	/* remove after 2.6.39 */
> -	if (capable(CAP_SYS_ADMIN))
> -		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> -		  "but no CAP_SYSLOG (deprecated and denied).\n");
> -	return -EPERM;
>  }
>  
>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3)
  2011-02-10 19:16 ` Linus Torvalds
  2011-02-10 22:43   ` Serge E. Hallyn
@ 2011-02-10 22:59   ` James Morris
  2011-02-11 16:32   ` Serge E. Hallyn
  2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
  3 siblings, 0 replies; 11+ messages in thread
From: James Morris @ 2011-02-10 22:59 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Serge E. Hallyn, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, Nick Bowler

On Thu, 10 Feb 2011, Linus Torvalds wrote:

> On Thu, Feb 10, 2011 at 6:40 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > Please apply.
> 
> Hmm. So I detest the duplication and the ugly resulting code.  It was
> a bit hard to follow before, now it's just nasty.
> 
> Why not make this all into some nicer helper functions instead -
> something simple like the attached?
>
> UNTESTED! I'm not going to apply it unless I get acks/tested-by's.

Reviewed-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3)
  2011-02-10 19:16 ` Linus Torvalds
  2011-02-10 22:43   ` Serge E. Hallyn
  2011-02-10 22:59   ` James Morris
@ 2011-02-11 16:32   ` Serge E. Hallyn
  2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
  3 siblings, 0 replies; 11+ messages in thread
From: Serge E. Hallyn @ 2011-02-11 16:32 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Serge E. Hallyn, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, James Morris, Nick Bowler

Quoting Linus Torvalds (torvalds@linux-foundation.org):
> On Thu, Feb 10, 2011 at 6:40 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> >
> > Please apply.
> 
> Hmm. So I detest the duplication and the ugly resulting code.  It was
> a bit hard to follow before, now it's just nasty.
> 
> Why not make this all into some nicer helper functions instead -
> something simple like the attached?
> 
> UNTESTED! I'm not going to apply it unless I get acks/tested-by's.

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Tested-by: Serge Hallyn <serge.hallyn@canonical.com>

I tested using klogctl with actions 10, 3 and 5.  Without
dmesg_restrict set, an unprivileged program could get the
size and read-all, but not clear the buffer.  With either
CAP_SYS_ADMIN or CAP_SYSLOG, it could do all three.  With
dmesg_restrict set, the capabilities were needed for all
actions.

Worked perfectly.

thanks,
-serge

>                                   Linus

>  kernel/printk.c |   54 +++++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 2ddbdc7..f51214c 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -262,25 +262,47 @@ int dmesg_restrict = 1;
>  int dmesg_restrict;
>  #endif
>  
> +static int syslog_action_restricted(int type)
> +{
> +	if (dmesg_restrict)
> +		return 1;
> +	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> +	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +	/*
> +	 * If this is from /proc/kmsg and we've already opened it, then we've
> +	 * already the capabilities checks at open time.
> +	 */
> +	if (from_file && type != SYSLOG_ACTION_OPEN)
> +		return 0;
> +
> +	if (syslog_action_restricted(type)) {
> +		if (capable(CAP_SYSLOG))
> +			return 0;
> +		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> +		if (capable(CAP_SYS_ADMIN)) {
> +			WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> +				 "but no CAP_SYSLOG (deprecated).\n");
> +			return 0;
> +		}
> +		return -EPERM;
> +	}
> +	return 0;
> +}
> +
>  int do_syslog(int type, char __user *buf, int len, bool from_file)
>  {
>  	unsigned i, j, limit, count;
>  	int do_clear = 0;
>  	char c;
> -	int error = 0;
> +	int error;
>  
> -	/*
> -	 * If this is from /proc/kmsg we only do the capabilities checks
> -	 * at open time.
> -	 */
> -	if (type == SYSLOG_ACTION_OPEN || !from_file) {
> -		if (dmesg_restrict && !capable(CAP_SYSLOG))
> -			goto warn; /* switch to return -EPERM after 2.6.39 */
> -		if ((type != SYSLOG_ACTION_READ_ALL &&
> -		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
> -		    !capable(CAP_SYSLOG))
> -			goto warn; /* switch to return -EPERM after 2.6.39 */
> -	}
> +	error = check_syslog_permissions(type, from_file);
> +	if (error)
> +		goto out;
>  
>  	error = security_syslog(type);
>  	if (error)
> @@ -423,12 +445,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	}
>  out:
>  	return error;
> -warn:
> -	/* remove after 2.6.39 */
> -	if (capable(CAP_SYS_ADMIN))
> -		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> -		  "but no CAP_SYSLOG (deprecated and denied).\n");
> -	return -EPERM;
>  }
>  
>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming
  2011-02-10 19:16 ` Linus Torvalds
                     ` (2 preceding siblings ...)
  2011-02-11 16:32   ` Serge E. Hallyn
@ 2011-08-03 16:48   ` Jonathan Nieder
  2011-08-04  1:28     ` James Morris
                       ` (3 more replies)
  3 siblings, 4 replies; 11+ messages in thread
From: Jonathan Nieder @ 2011-08-03 16:48 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Serge E. Hallyn, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, James Morris, Nick Bowler

syslog-ng versions in active use assume that CAP_SYS_ADMIN is
sufficent to access syslog.  As a result, ever since CAP_SYSLOG was
introduce in v2.6.38-rc1~429^2~18 (security: Define CAP_SYSLOG,
2010-11-25), they have triggered a warning, complete with kernel
backtrace.

v2.6.38-rc5~46 (cap_syslog: accept CAP_SYS_ADMIN for now, 2011-02-10)
made things a little better by removing the regression in behavior,
just keeping the WARN_ONCE.  But still, this is a warning that adds
many lines to syslog, sets a taint flag, and alarms sysadmins when
nothing worse has happened than use of an old userspace with a recent
kernel.

Convert the WARN_ONCE to a printk_once to avoid this while continuing
to give userspace developers a hint that this is an unwanted
backward-compatibility feature and won't be around forever.

Reported-by: Ralf Hildebrandt <ralf.hildebrandt@charite.de>
Reported-by: Niels <zorglub_olsen@hotmail.com>
Reported-by: Paweł Sikora <pluto@agmk.net>
Liked-by: Gergely Nagy <algernon@madhouse-project.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Nothing urgent about this, but it seems to be a frequently[1] reported[2]
source of unnecessary worry.  Thoughts?

[1] http://thread.gmane.org/gmane.linux.kernel/1145040
[2] http://thread.gmane.org/gmane.linux.kernel/1153808

Context: http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=32;bug=636501

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

diff --git a/kernel/printk.c b/kernel/printk.c
index 37dff342..db64c951 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -318,7 +318,8 @@ static int check_syslog_permissions(int type, bool from_file)
 			return 0;
 		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
 		if (capable(CAP_SYS_ADMIN)) {
-			WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
+			printk_once(KERN_WARNING
+				 "Attempt to access syslog with CAP_SYS_ADMIN "
 				 "but no CAP_SYSLOG (deprecated).\n");
 			return 0;
 		}
-- 
1.7.6


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming
  2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
@ 2011-08-04  1:28     ` James Morris
  2011-08-04  4:39     ` Serge E. Hallyn
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: James Morris @ 2011-08-04  1:28 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Linus Torvalds, Serge E. Hallyn, Gergely Nagy, david, Alan Cox,
	Marc Koschewski, lkml, Nick Bowler

On Wed, 3 Aug 2011, Jonathan Nieder wrote:

> Convert the WARN_ONCE to a printk_once to avoid this while continuing
> to give userspace developers a hint that this is an unwanted
> backward-compatibility feature and won't be around forever.

Seems reasonable to me.

-- 
James Morris
<jmorris@namei.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming
  2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
  2011-08-04  1:28     ` James Morris
@ 2011-08-04  4:39     ` Serge E. Hallyn
  2011-08-05 13:45     ` James Morris
  2011-08-05 18:50     ` Linus Torvalds
  3 siblings, 0 replies; 11+ messages in thread
From: Serge E. Hallyn @ 2011-08-04  4:39 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Linus Torvalds, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, James Morris, Nick Bowler

Quoting Jonathan Nieder (jrnieder@gmail.com):
> syslog-ng versions in active use assume that CAP_SYS_ADMIN is
> sufficent to access syslog.  As a result, ever since CAP_SYSLOG was
> introduce in v2.6.38-rc1~429^2~18 (security: Define CAP_SYSLOG,
> 2010-11-25), they have triggered a warning, complete with kernel
> backtrace.
> 
> v2.6.38-rc5~46 (cap_syslog: accept CAP_SYS_ADMIN for now, 2011-02-10)
> made things a little better by removing the regression in behavior,
> just keeping the WARN_ONCE.  But still, this is a warning that adds
> many lines to syslog, sets a taint flag, and alarms sysadmins when

Sets the taint flag?  That's a bit over the top, so:

> nothing worse has happened than use of an old userspace with a recent
> kernel.
> 
> Convert the WARN_ONCE to a printk_once to avoid this while continuing
> to give userspace developers a hint that this is an unwanted
> backward-compatibility feature and won't be around forever.
> 
> Reported-by: Ralf Hildebrandt <ralf.hildebrandt@charite.de>
> Reported-by: Niels <zorglub_olsen@hotmail.com>
> Reported-by: Paweł Sikora <pluto@agmk.net>
> Liked-by: Gergely Nagy <algernon@madhouse-project.org>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

thanks,
-serge

> ---
> Hi,
> 
> Nothing urgent about this, but it seems to be a frequently[1] reported[2]
> source of unnecessary worry.  Thoughts?
> 
> [1] http://thread.gmane.org/gmane.linux.kernel/1145040
> [2] http://thread.gmane.org/gmane.linux.kernel/1153808
> 
> Context: http://bugs.debian.org/cgi-bin/bugreport.cgi?msg=32;bug=636501
> 
>  kernel/printk.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 37dff342..db64c951 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -318,7 +318,8 @@ static int check_syslog_permissions(int type, bool from_file)
>  			return 0;
>  		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>  		if (capable(CAP_SYS_ADMIN)) {
> -			WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> +			printk_once(KERN_WARNING
> +				 "Attempt to access syslog with CAP_SYS_ADMIN "
>  				 "but no CAP_SYSLOG (deprecated).\n");
>  			return 0;
>  		}
> -- 
> 1.7.6
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming
  2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
  2011-08-04  1:28     ` James Morris
  2011-08-04  4:39     ` Serge E. Hallyn
@ 2011-08-05 13:45     ` James Morris
  2011-08-05 18:50     ` Linus Torvalds
  3 siblings, 0 replies; 11+ messages in thread
From: James Morris @ 2011-08-05 13:45 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Linus Torvalds, Serge E. Hallyn, Gergely Nagy, david, Alan Cox,
	Marc Koschewski, lkml, Nick Bowler

On Wed, 3 Aug 2011, Jonathan Nieder wrote:

> Convert the WARN_ONCE to a printk_once to avoid this while continuing
> to give userspace developers a hint that this is an unwanted
> backward-compatibility feature and won't be around forever.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next-queue

-- 
James Morris
<jmorris@namei.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming
  2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
                       ` (2 preceding siblings ...)
  2011-08-05 13:45     ` James Morris
@ 2011-08-05 18:50     ` Linus Torvalds
  2011-08-08  4:22       ` [PATCH v2] cap_syslog: don't use WARN_ONCE for CAP_SYS_ADMIN deprecation warning Jonathan Nieder
  3 siblings, 1 reply; 11+ messages in thread
From: Linus Torvalds @ 2011-08-05 18:50 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Serge E. Hallyn, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, James Morris, Nick Bowler

On Wed, Aug 3, 2011 at 6:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Convert the WARN_ONCE to a printk_once to avoid this while continuing
> to give userspace developers a hint that this is an unwanted
> backward-compatibility feature and won't be around forever.

The problem with this one is that converting away from a warning
doesn't just get rid of all the ugliness, it gets rid of some of the
*useful* information in the warning too.

In particular, when you have a warning like "Attempting to access
syslog", you'd better tell people *who* is attempting to access
syslog, so that they can fix it or complain to the right person.

And the warning does show what process it is (in addition to
stacktrace, tainting etc that we don't want). Your printk_once()
conversion does not.

         Linus

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] cap_syslog: don't use WARN_ONCE for CAP_SYS_ADMIN deprecation warning
  2011-08-05 18:50     ` Linus Torvalds
@ 2011-08-08  4:22       ` Jonathan Nieder
  0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Nieder @ 2011-08-08  4:22 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Serge E. Hallyn, Gergely Nagy, david, Alan Cox, Marc Koschewski,
	lkml, James Morris, Nick Bowler

syslog-ng versions before 3.3.0beta1 (2011-05-12) assume that
CAP_SYS_ADMIN is sufficient to access syslog, so ever since CAP_SYSLOG
was introduced (2010-11-25) they have triggered a warning.
v2.6.38-rc5~46 (cap_syslog: accept CAP_SYS_ADMIN for now, 2011-02-10)
improved matters a little by making syslog-ng work again, just keeping
the WARN_ONCE().  But still, this is a warning that writes a stack
trace we don't care about to syslog, sets a taint flag, and alarms
sysadmins when nothing worse has happened than use of an old userspace
with a recent kernel.

Convert the WARN_ONCE to a printk_once to avoid that while continuing
to give userspace developers a hint that this is an unwanted
backward-compatibility feature and won't be around forever.

Reported-by: Ralf Hildebrandt <ralf.hildebrandt@charite.de>
Reported-by: Niels <zorglub_olsen@hotmail.com>
Reported-by: Paweł Sikora <pluto@agmk.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Liked-by: Gergely Nagy <algernon@madhouse-project.org>
Acked-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <jmorris@namei.org>
---
Linus Torvalds wrote:

> The problem with this one is that converting away from a warning
> doesn't just get rid of all the ugliness, it gets rid of some of the
> *useful* information in the warning too.
>
> In particular, when you have a warning like "Attempting to access
> syslog", you'd better tell people *who* is attempting to access
> syslog, so that they can fix it or complain to the right person.

True.  I can't imagine people wanting to know much more than the
process's command line and pid, which is basically what the WARN_ONCE
gives (though it also gives registers).  Here's an updated patch.

I've carried over the acks from last time; hopefully that's okay.
Thanks for your help.

 kernel/printk.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 37dff342..836a2ae0 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -318,8 +318,10 @@ static int check_syslog_permissions(int type, bool from_file)
 			return 0;
 		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
 		if (capable(CAP_SYS_ADMIN)) {
-			WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
-				 "but no CAP_SYSLOG (deprecated).\n");
+			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;
 		}
 		return -EPERM;
-- 
1.7.6


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2011-08-08  4:22 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-10 14:40 [PATCH 1/1] cap_syslog: don't refuse cap_sys_admin for now (v3) Serge E. Hallyn
2011-02-10 19:16 ` Linus Torvalds
2011-02-10 22:43   ` Serge E. Hallyn
2011-02-10 22:59   ` James Morris
2011-02-11 16:32   ` Serge E. Hallyn
2011-08-03 16:48   ` [PATCH/RFC] cap_syslog: make CAP_SYS_ADMIN deprecation notice less alarming Jonathan Nieder
2011-08-04  1:28     ` James Morris
2011-08-04  4:39     ` Serge E. Hallyn
2011-08-05 13:45     ` James Morris
2011-08-05 18:50     ` Linus Torvalds
2011-08-08  4:22       ` [PATCH v2] cap_syslog: don't use WARN_ONCE for CAP_SYS_ADMIN deprecation warning Jonathan Nieder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).