All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eric Paris <eparis@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: sds@tycho.nsa.gov, jmorris@namei.org, morgan@kernel.org,
	serue@us.ibm.com, selinux@tycho.nsa.gov
Subject: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
Date: Tue, 30 Sep 2008 09:55:46 -0400	[thread overview]
Message-ID: <1222782946.28251.63.camel@localhost.localdomain> (raw)

The firegl ATI/AMD closed source proprietary driver has some 2,733
panics registered on kerneloops.org and recently SELinux was blamed for
BUGing because of this driver.

http://www.kerneloops.org/search.php?search=firegl_ioctl&btnG=Function+Search

Basically it looks like the firegl_ioctl() function is calling capable()
with a huge value.  cap_capable dereferences an array (64 bits long) WAY
off the end.  I know a specific example would be referencing the array
c.cap[0x7B4BB01].  As dumb luck would have it occasionally this doesn't
pagefault / panic and so we get to selinux code which has a clean check
for values which are obviously too large and BUG().

This patch adds a WARN_ONCE() to cap_capable() so we will stop
dereferencing random spots of memory and will cleanly tell the obviously
broken driver that it doesn't have that ridiculous permissions.  No idea
if the driver is going to handle EPERM but anything that calls capable
and doesn't expect a denial has got to be the worst piece of code ever
written.....  I could return EINVAL, but I think its clear that noone
has capabilities over 64 so clearly they don't have that permission.

This 'could' be considered a regression since 2.6.24.  Neither SELinux
nor the capabilities system had a problem with ginormous request values
until we got 64 bit support, although this is OBVIOUSLY a bug with the
out of tree closed source driver....

Oh yeah, and if anyone knows people at ATI/AMD tell them to fix their
broken driver....

Signed-off-by: Eric Paris <eparis@redhat.com>

---

 security/commoncap.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index e4c4b3f..92715e7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -50,6 +50,11 @@ EXPORT_SYMBOL(cap_netlink_recv);
  */
 int cap_capable (struct task_struct *tsk, int cap)
 {
+	if (unlikely(!cap_valid(cap))) {
+		WARN_ONCE(1, "Invalid capability:%d\n", cap);
+		return -EPERM;
+	}
+
 	/* Derived from include/linux/sched.h:capable. */
 	if (cap_raised(tsk->cap_effective, cap))
 		return 0;



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

WARNING: multiple messages have this Message-ID (diff)
From: Eric Paris <eparis@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: sds@tycho.nsa.gov, jmorris@namei.org, morgan@kernel.org,
	serue@us.ibm.com, selinux@tycho.nsa.gov
Subject: [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic
Date: Tue, 30 Sep 2008 09:55:46 -0400	[thread overview]
Message-ID: <1222782946.28251.63.camel@localhost.localdomain> (raw)

The firegl ATI/AMD closed source proprietary driver has some 2,733
panics registered on kerneloops.org and recently SELinux was blamed for
BUGing because of this driver.

http://www.kerneloops.org/search.php?search=firegl_ioctl&btnG=Function+Search

Basically it looks like the firegl_ioctl() function is calling capable()
with a huge value.  cap_capable dereferences an array (64 bits long) WAY
off the end.  I know a specific example would be referencing the array
c.cap[0x7B4BB01].  As dumb luck would have it occasionally this doesn't
pagefault / panic and so we get to selinux code which has a clean check
for values which are obviously too large and BUG().

This patch adds a WARN_ONCE() to cap_capable() so we will stop
dereferencing random spots of memory and will cleanly tell the obviously
broken driver that it doesn't have that ridiculous permissions.  No idea
if the driver is going to handle EPERM but anything that calls capable
and doesn't expect a denial has got to be the worst piece of code ever
written.....  I could return EINVAL, but I think its clear that noone
has capabilities over 64 so clearly they don't have that permission.

This 'could' be considered a regression since 2.6.24.  Neither SELinux
nor the capabilities system had a problem with ginormous request values
until we got 64 bit support, although this is OBVIOUSLY a bug with the
out of tree closed source driver....

Oh yeah, and if anyone knows people at ATI/AMD tell them to fix their
broken driver....

Signed-off-by: Eric Paris <eparis@redhat.com>

---

 security/commoncap.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index e4c4b3f..92715e7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -50,6 +50,11 @@ EXPORT_SYMBOL(cap_netlink_recv);
  */
 int cap_capable (struct task_struct *tsk, int cap)
 {
+	if (unlikely(!cap_valid(cap))) {
+		WARN_ONCE(1, "Invalid capability:%d\n", cap);
+		return -EPERM;
+	}
+
 	/* Derived from include/linux/sched.h:capable. */
 	if (cap_raised(tsk->cap_effective, cap))
 		return 0;



             reply	other threads:[~2008-09-30 13:55 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-09-30 13:55 Eric Paris [this message]
2008-09-30 13:55 ` [PATCH] capability: WARN when invalid capability is requested rather than BUG/panic Eric Paris
2008-09-30 14:23 ` James Morris
2008-09-30 14:23   ` James Morris
2008-09-30 14:36   ` Eric Paris
2008-09-30 14:36     ` Eric Paris
2008-09-30 15:38     ` Serge E. Hallyn
2008-09-30 15:38       ` Serge E. Hallyn
2008-09-30 16:07       ` Eric Paris
2008-09-30 16:07         ` Eric Paris
2008-09-30 16:28         ` Serge E. Hallyn
2008-09-30 16:28           ` Serge E. Hallyn
2008-09-30 17:22           ` Eric Paris
2008-09-30 17:22             ` Eric Paris
2008-09-30 17:28             ` Arjan van de Ven
2008-10-01 15:32               ` Eric Paris
2008-10-01 15:32                 ` Eric Paris
2008-10-01 15:39                 ` Arjan van de Ven
2008-10-01 15:44                 ` Serge E. Hallyn
2008-10-01 15:44                   ` Serge E. Hallyn
2008-10-05  1:30           ` Andrew G. Morgan
2008-10-05  1:30             ` Andrew G. Morgan
     [not found] <bhO5y-S0-29@gated-at.bofh.it>
     [not found] ` <bhOyr-1kZ-5@gated-at.bofh.it>
     [not found]   ` <bhOyr-1kZ-3@gated-at.bofh.it>
     [not found]     ` <bhPuC-2yN-5@gated-at.bofh.it>
     [not found]       ` <bhPXy-3jl-13@gated-at.bofh.it>
     [not found]         ` <bhQh0-3CK-9@gated-at.bofh.it>
     [not found]           ` <bhRd4-4RS-9@gated-at.bofh.it>
     [not found]             ` <bhRd8-4RS-27@gated-at.bofh.it>
     [not found]               ` <bibY4-6WP-13@gated-at.bofh.it>
2008-10-01 19:36                 ` Bodo Eggert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1222782946.28251.63.camel@localhost.localdomain \
    --to=eparis@redhat.com \
    --cc=jmorris@namei.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=morgan@kernel.org \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@tycho.nsa.gov \
    --cc=serue@us.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.