[PATCH 0/2] pci: refer to LSM when accessing device dependent config space

[PATCH 0/2] pci: refer to LSM when accessing device dependent config space

[Chris Wright]

Short patch series to make sure LSM gets visibility into attempt to
access device dependent config space.  Fixes a buglet I introduced in
de139a3 ("pci: check caps from sysfs file open to read device dependent
config space")

Chris Wright (2):
  security: add cred argument to security_capable()
  pci: use security_capable() when checking capablities during config
    space read

 drivers/pci/pci-sysfs.c  |    3 ++-
 include/linux/security.h |    6 +++---
 kernel/capability.c      |    2 +-
 security/security.c      |    5 ++---
 4 files changed, 8 insertions(+), 8 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] security: add cred argument to security_capable()

[Chris Wright]

Expand security_capable() to include cred, so that it can be usable in a
wider range of call sites.

Cc: James Morris <jmorris@namei.org>
Cc: Eric Paris <eparis@redhat.com>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---

 include/linux/security.h |    6 +++---
 kernel/capability.c      |    2 +-
 security/security.c      |    5 ++---
 3 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index c642bb8..b2b7f97 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1662,7 +1662,7 @@ int security_capset(struct cred *new, const struct cred *old,
 		    const kernel_cap_t *effective,
 		    const kernel_cap_t *inheritable,
 		    const kernel_cap_t *permitted);
-int security_capable(int cap);
+int security_capable(const struct cred *cred, int cap);
 int security_real_capable(struct task_struct *tsk, int cap);
 int security_real_capable_noaudit(struct task_struct *tsk, int cap);
 int security_sysctl(struct ctl_table *table, int op);
@@ -1856,9 +1856,9 @@ static inline int security_capset(struct cred *new,
 	return cap_capset(new, old, effective, inheritable, permitted);
 }
 
-static inline int security_capable(int cap)
+static inline int security_capable(const struct cred *cred, int cap)
 {
-	return cap_capable(current, current_cred(), cap, SECURITY_CAP_AUDIT);
+	return cap_capable(current, cred, cap, SECURITY_CAP_AUDIT);
 }
 
 static inline int security_real_capable(struct task_struct *tsk, int cap)
diff --git a/kernel/capability.c b/kernel/capability.c
index 2f05303..9e9385f 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -306,7 +306,7 @@ int capable(int cap)
 		BUG();
 	}
 
-	if (security_capable(cap) == 0) {
+	if (security_capable(current_cred(), cap) == 0) {
 		current->flags |= PF_SUPERPRIV;
 		return 1;
 	}
diff --git a/security/security.c b/security/security.c
index 739e403..7b7308a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -154,10 +154,9 @@ int security_capset(struct cred *new, const struct cred *old,
 				    effective, inheritable, permitted);
 }
 
-int security_capable(int cap)
+int security_capable(const struct cred *cred, int cap)
 {
-	return security_ops->capable(current, current_cred(), cap,
-				     SECURITY_CAP_AUDIT);
+	return security_ops->capable(current, cred, cap, SECURITY_CAP_AUDIT);
 }
 
 int security_real_capable(struct task_struct *tsk, int cap)
-- 
1.7.3.4

[PATCH 2/2] pci: use security_capable() when checking capablities during config space read

[Chris Wright]

Eric Paris noted that commit de139a3 ("pci: check caps from sysfs file
open to read device dependent config space") caused the capability check
to bypass security modules and potentially auditing.  Rectify this by
calling security_capable() when checking the open file's capabilities
for config space reads.

Cc: Eric Paris <eparis@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/pci-sysfs.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8ecaac9..f7771f3 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/capability.h>
+#include <linux/security.h>
 #include <linux/pci-aspm.h>
 #include <linux/slab.h>
 #include "pci.h"
@@ -368,7 +369,7 @@ pci_read_config(struct file *filp, struct kobject *kobj,
 	u8 *data = (u8*) buf;
 
 	/* Several chips lock up trying to read undefined config space */
-	if (cap_raised(filp->f_cred->cap_effective, CAP_SYS_ADMIN)) {
+	if (security_capable(filp->f_cred, CAP_SYS_ADMIN)) {
 		size = dev->cfg_size;
 	} else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 		size = 128;
-- 
1.7.3.4

Re: [PATCH 2/2] pci: use security_capable() when checking capablities during config space read

[James Morris]

On Wed, 9 Feb 2011, Chris Wright wrote:

> Eric Paris noted that commit de139a3 ("pci: check caps from sysfs file
> open to read device dependent config space") caused the capability check
> to bypass security modules and potentially auditing.  Rectify this by
> calling security_capable() when checking the open file's capabilities
> for config space reads.

What about these other users of cap_raised?

drivers/block/drbd/drbd_nl.c:   if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
drivers/md/dm-log-userspace-transfer.c: if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
drivers/staging/pohmelfs/config.c:      if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
drivers/video/uvesafb.c:        if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))


Also, should this have a reported-by for Eric ?

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] security: add cred argument to security_capable()

[Serge E. Hallyn]

Quoting Chris Wright (chrisw@sous-sol.org):
> Expand security_capable() to include cred, so that it can be usable in a
> wider range of call sites.
> 
> Cc: James Morris <jmorris@namei.org>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

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

Thanks for cc:ing me, Chris.

Please do cc: me on any patches which exploit this.  Sending
current_cred() is fine, but of course sending another cred
can be trickier.  Additionally, it'll affect my userns patchset,
so I'd just like to keep abreast of what's happening.

thanks,
-serge

> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
> 
>  include/linux/security.h |    6 +++---
>  kernel/capability.c      |    2 +-
>  security/security.c      |    5 ++---
>  3 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c642bb8..b2b7f97 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1662,7 +1662,7 @@ int security_capset(struct cred *new, const struct cred *old,
>  		    const kernel_cap_t *effective,
>  		    const kernel_cap_t *inheritable,
>  		    const kernel_cap_t *permitted);
> -int security_capable(int cap);
> +int security_capable(const struct cred *cred, int cap);
>  int security_real_capable(struct task_struct *tsk, int cap);
>  int security_real_capable_noaudit(struct task_struct *tsk, int cap);
>  int security_sysctl(struct ctl_table *table, int op);
> @@ -1856,9 +1856,9 @@ static inline int security_capset(struct cred *new,
>  	return cap_capset(new, old, effective, inheritable, permitted);
>  }
>  
> -static inline int security_capable(int cap)
> +static inline int security_capable(const struct cred *cred, int cap)
>  {
> -	return cap_capable(current, current_cred(), cap, SECURITY_CAP_AUDIT);
> +	return cap_capable(current, cred, cap, SECURITY_CAP_AUDIT);
>  }
>  
>  static inline int security_real_capable(struct task_struct *tsk, int cap)
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 2f05303..9e9385f 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -306,7 +306,7 @@ int capable(int cap)
>  		BUG();
>  	}
>  
> -	if (security_capable(cap) == 0) {
> +	if (security_capable(current_cred(), cap) == 0) {
>  		current->flags |= PF_SUPERPRIV;
>  		return 1;
>  	}
> diff --git a/security/security.c b/security/security.c
> index 739e403..7b7308a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -154,10 +154,9 @@ int security_capset(struct cred *new, const struct cred *old,
>  				    effective, inheritable, permitted);
>  }
>  
> -int security_capable(int cap)
> +int security_capable(const struct cred *cred, int cap)
>  {
> -	return security_ops->capable(current, current_cred(), cap,
> -				     SECURITY_CAP_AUDIT);
> +	return security_ops->capable(current, cred, cap, SECURITY_CAP_AUDIT);
>  }
>  
>  int security_real_capable(struct task_struct *tsk, int cap)
> -- 
> 1.7.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] security: add cred argument to security_capable()

[Casey Schaufler]

On 2/9/2011 10:11 PM, Chris Wright wrote:
> Expand security_capable() to include cred, so that it can be usable in a
> wider range of call sites.

I'll bite. What to plan to use this for? I wouldn't see this getting
accepted on its own without a user. I don't see anything wrong with
the change other than that it is not used by anything.


> Cc: James Morris <jmorris@namei.org>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>
>  include/linux/security.h |    6 +++---
>  kernel/capability.c      |    2 +-
>  security/security.c      |    5 ++---
>  3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index c642bb8..b2b7f97 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1662,7 +1662,7 @@ int security_capset(struct cred *new, const struct cred *old,
>  		    const kernel_cap_t *effective,
>  		    const kernel_cap_t *inheritable,
>  		    const kernel_cap_t *permitted);
> -int security_capable(int cap);
> +int security_capable(const struct cred *cred, int cap);
>  int security_real_capable(struct task_struct *tsk, int cap);
>  int security_real_capable_noaudit(struct task_struct *tsk, int cap);
>  int security_sysctl(struct ctl_table *table, int op);
> @@ -1856,9 +1856,9 @@ static inline int security_capset(struct cred *new,
>  	return cap_capset(new, old, effective, inheritable, permitted);
>  }
>  
> -static inline int security_capable(int cap)
> +static inline int security_capable(const struct cred *cred, int cap)
>  {
> -	return cap_capable(current, current_cred(), cap, SECURITY_CAP_AUDIT);
> +	return cap_capable(current, cred, cap, SECURITY_CAP_AUDIT);
>  }
>  
>  static inline int security_real_capable(struct task_struct *tsk, int cap)
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 2f05303..9e9385f 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -306,7 +306,7 @@ int capable(int cap)
>  		BUG();
>  	}
>  
> -	if (security_capable(cap) == 0) {
> +	if (security_capable(current_cred(), cap) == 0) {
>  		current->flags |= PF_SUPERPRIV;
>  		return 1;
>  	}
> diff --git a/security/security.c b/security/security.c
> index 739e403..7b7308a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -154,10 +154,9 @@ int security_capset(struct cred *new, const struct cred *old,
>  				    effective, inheritable, permitted);
>  }
>  
> -int security_capable(int cap)
> +int security_capable(const struct cred *cred, int cap)
>  {
> -	return security_ops->capable(current, current_cred(), cap,
> -				     SECURITY_CAP_AUDIT);
> +	return security_ops->capable(current, cred, cap, SECURITY_CAP_AUDIT);
>  }
>  
>  int security_real_capable(struct task_struct *tsk, int cap)

Re: [PATCH 1/2] security: add cred argument to security_capable()

[Chris Wright]

* Casey Schaufler (casey@schaufler-ca.com) wrote:
> On 2/9/2011 10:11 PM, Chris Wright wrote:
> > Expand security_capable() to include cred, so that it can be usable in a
> > wider range of call sites.
> 
> I'll bite. What to plan to use this for? I wouldn't see this getting
> accepted on its own without a user. I don't see anything wrong with
> the change other than that it is not used by anything.

Casey, the user is in 2/2.

https://lkml.org/lkml/2011/2/10/16

Re: [PATCH 1/2] security: add cred argument to security_capable()

[Casey Schaufler]

On 2/10/2011 8:08 AM, Chris Wright wrote:
> * Casey Schaufler (casey@schaufler-ca.com) wrote:
>> On 2/9/2011 10:11 PM, Chris Wright wrote:
>>> Expand security_capable() to include cred, so that it can be usable in a
>>> wider range of call sites.
>> I'll bite. What to plan to use this for? I wouldn't see this getting
>> accepted on its own without a user. I don't see anything wrong with
>> the change other than that it is not used by anything.
> Casey, the user is in 2/2.

Which didn't show up on the LSM list. I see it now, but
had to go looking. Ok, no worries

> https://lkml.org/lkml/2011/2/10/16
>
>

[PATCH 2/2 v2] pci: use security_capable() when checking capablities during config space read

[Chris Wright]

* James Morris (jmorris@namei.org) wrote:
> What about these other users of cap_raised?
> 
> drivers/block/drbd/drbd_nl.c:   if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
> drivers/md/dm-log-userspace-transfer.c: if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> drivers/staging/pohmelfs/config.c:      if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> drivers/video/uvesafb.c:        if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))

Those are a security_netlink_recv() variant.  They should be converted
although makes sense as a different patchset.

> Also, should this have a reported-by for Eric ?

Yes it should, thanks.  Below is patch with Reported-by added (seemed
overkill to respin the series; holler if that's perferred).

thanks,
-chris
---

From: Chris Wright <chrisw@sous-sol.org>
Subject: [PATCH 2/2 v2] pci: use security_capable() when checking capablities during config space read

Eric Paris noted that commit de139a3 ("pci: check caps from sysfs file
open to read device dependent config space") caused the capability check
to bypass security modules and potentially auditing.  Rectify this by
calling security_capable() when checking the open file's capabilities
for config space reads.

Reported-by: Eric Paris <eparis@redhat.com>
Cc: Eric Paris <eparis@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: linux-pci@vger.kernel.org
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
 drivers/pci/pci-sysfs.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 8ecaac9..f7771f3 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/capability.h>
+#include <linux/security.h>
 #include <linux/pci-aspm.h>
 #include <linux/slab.h>
 #include "pci.h"
@@ -368,7 +369,7 @@ pci_read_config(struct file *filp, struct kobject *kobj,
 	u8 *data = (u8*) buf;
 
 	/* Several chips lock up trying to read undefined config space */
-	if (cap_raised(filp->f_cred->cap_effective, CAP_SYS_ADMIN)) {
+	if (security_capable(filp->f_cred, CAP_SYS_ADMIN)) {
 		size = dev->cfg_size;
 	} else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
 		size = 128;

Re: [PATCH 0/2] pci: refer to LSM when accessing device dependent config space

[James Morris]

On Wed, 9 Feb 2011, Chris Wright wrote:

> Short patch series to make sure LSM gets visibility into attempt to
> access device dependent config space.  Fixes a buglet I introduced in
> de139a3 ("pci: check caps from sysfs file open to read device dependent
> config space")
> 
> Chris Wright (2):
>   security: add cred argument to security_capable()
>   pci: use security_capable() when checking capablities during config
>     space read

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



-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/2 v2] pci: use security_capable() when checking capablities during config space read

[Eric Paris]

On Thu, 2011-02-10 at 15:58 -0800, Chris Wright wrote:
> * James Morris (jmorris@namei.org) wrote:
> > What about these other users of cap_raised?
> > 
> > drivers/block/drbd/drbd_nl.c:   if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
> > drivers/md/dm-log-userspace-transfer.c: if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> > drivers/staging/pohmelfs/config.c:      if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> > drivers/video/uvesafb.c:        if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> 
> Those are a security_netlink_recv() variant.  They should be converted
> although makes sense as a different patchset.
> 
> > Also, should this have a reported-by for Eric ?
> 
> Yes it should, thanks.  Below is patch with Reported-by added (seemed
> overkill to respin the series; holler if that's perferred).

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

on both of them.

-eric

Re: [PATCH 2/2 v2] pci: use security_capable() when checking capablities during config space read

[Jesse Barnes]

On Thu, 10 Feb 2011 15:58:56 -0800
Chris Wright <chrisw@sous-sol.org> wrote:

> * James Morris (jmorris@namei.org) wrote:
> > What about these other users of cap_raised?
> > 
> > drivers/block/drbd/drbd_nl.c:   if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
> > drivers/md/dm-log-userspace-transfer.c: if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> > drivers/staging/pohmelfs/config.c:      if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> > drivers/video/uvesafb.c:        if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
> 
> Those are a security_netlink_recv() variant.  They should be converted
> although makes sense as a different patchset.
> 
> > Also, should this have a reported-by for Eric ?
> 
> Yes it should, thanks.  Below is patch with Reported-by added (seemed
> overkill to respin the series; holler if that's perferred).
> 
> thanks,
> -chris
> ---
> 
> From: Chris Wright <chrisw@sous-sol.org>
> Subject: [PATCH 2/2 v2] pci: use security_capable() when checking capablities during config space read
> 
> Eric Paris noted that commit de139a3 ("pci: check caps from sysfs file
> open to read device dependent config space") caused the capability check
> to bypass security modules and potentially auditing.  Rectify this by
> calling security_capable() when checking the open file's capabilities
> for config space reads.
> 
> Reported-by: Eric Paris <eparis@redhat.com>
> Cc: Eric Paris <eparis@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: linux-pci@vger.kernel.org
> Signed-off-by: Chris Wright <chrisw@sous-sol.org>
> ---
>  drivers/pci/pci-sysfs.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Sorry for the late reply, but this is fine with me.  Should probably
just get pushed along with the change to security_capable (assuming
that hasn't been done already).

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center