Re: [RFC PATCH] selinux: always return a value from the netport/netnode/netif caches
On Thursday, April 14, 2016 10:09:25 AM Stephen Smalley wrote:
> On 04/13/2016 05:37 PM, Paul Moore wrote:
> > From: Paul Moore
> >
> > Even if we are under memory pressure and can't allocate a new cache
> > node we can still return the port/node/iface value we looked up from
> > the policy.
> >
> > Reported-by: Greg
> > Signed-off-by: Paul Moore
> > ---
> >
> > security/selinux/netif.c | 35 +--
> > security/selinux/netnode.c | 31 +--
> > security/selinux/netport.c | 19 ---
> > 3 files changed, 38 insertions(+), 47 deletions(-)
> >
> > diff --git a/security/selinux/netif.c b/security/selinux/netif.c
> > index e607b44..5c3bfa4 100644
> > --- a/security/selinux/netif.c
> > +++ b/security/selinux/netif.c
> > @@ -91,18 +91,16 @@ static inline struct sel_netif *sel_netif_find(const
> > struct net *ns,>
> > * zero on success, negative values on failure.
> > *
> > */
> >
> > -static int sel_netif_insert(struct sel_netif *netif)
> > +static void sel_netif_insert(struct sel_netif *netif)
> >
> > {
> >
> > int idx;
> >
> > if (sel_netif_total >= SEL_NETIF_HASH_MAX)
> >
> > - return -ENOSPC;
> > + return;
>
> This will leak netif (new in the caller). Looks like the other
> sel_*_insert() functions handle freeing of the entry if we hit the limit.
Yes, good catch.
For a while now I thought we would be better off if we consolidated the
different network object caches into one small cache implementation with
object specific callouts (hash, match, etc.) and cache instances. There is so
much duplicated code between these three and there really is no need for it.
Perhaps I'll play with that this weekend if I get some time.
--
paul moore
security @ redhat
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC PATCH] selinux: always return a value from the netport/netnode/netif caches
On 04/13/2016 05:37 PM, Paul Moore wrote:
> From: Paul Moore
>
> Even if we are under memory pressure and can't allocate a new cache
> node we can still return the port/node/iface value we looked up from
> the policy.
>
> Reported-by: Greg
> Signed-off-by: Paul Moore
> ---
> security/selinux/netif.c | 35 +--
> security/selinux/netnode.c | 31 +--
> security/selinux/netport.c | 19 ---
> 3 files changed, 38 insertions(+), 47 deletions(-)
>
> diff --git a/security/selinux/netif.c b/security/selinux/netif.c
> index e607b44..5c3bfa4 100644
> --- a/security/selinux/netif.c
> +++ b/security/selinux/netif.c
> @@ -91,18 +91,16 @@ static inline struct sel_netif *sel_netif_find(const
> struct net *ns,
> * zero on success, negative values on failure.
> *
> */
> -static int sel_netif_insert(struct sel_netif *netif)
> +static void sel_netif_insert(struct sel_netif *netif)
> {
> int idx;
>
> if (sel_netif_total >= SEL_NETIF_HASH_MAX)
> - return -ENOSPC;
> + return;
This will leak netif (new in the caller). Looks like the other
sel_*_insert() functions handle freeing of the entry if we hit the limit.
>
> idx = sel_netif_hashfn(netif->nsec.ns, netif->nsec.ifindex);
> list_add_rcu(&netif->list, &sel_netif_hash[idx]);
> sel_netif_total++;
> -
> - return 0;
> }
>
> /**
> @@ -135,7 +133,7 @@ static void sel_netif_destroy(struct sel_netif *netif)
> */
> static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid)
> {
> - int ret;
> + int ret = 0;
> struct sel_netif *netif;
> struct sel_netif *new = NULL;
> struct net_device *dev;
> @@ -155,34 +153,27 @@ static int sel_netif_sid_slow(struct net *ns, int
> ifindex, u32 *sid)
> netif = sel_netif_find(ns, ifindex);
> if (netif != NULL) {
> *sid = netif->nsec.sid;
> - ret = 0;
> goto out;
> }
> - new = kzalloc(sizeof(*new), GFP_ATOMIC);
> - if (new == NULL) {
> - ret = -ENOMEM;
> + ret = security_netif_sid(dev->name, sid);
> + if (ret != 0) {
> + printk(KERN_WARNING
> +"SELinux: failure in sel_netif_sid_slow(),"
> +" unable to determine network interface label (%d)\n",
> +ifindex);
> goto out;
> }
> - ret = security_netif_sid(dev->name, &new->nsec.sid);
> - if (ret != 0)
> + new = kzalloc(sizeof(*new), GFP_ATOMIC);
> + if (new == NULL)
> goto out;
> new->nsec.ns = ns;
> new->nsec.ifindex = ifindex;
> - ret = sel_netif_insert(new);
> - if (ret != 0)
> - goto out;
> - *sid = new->nsec.sid;
> + new->nsec.sid = *sid;
> + sel_netif_insert(new);
>
> out:
> spin_unlock_bh(&sel_netif_lock);
> dev_put(dev);
> - if (unlikely(ret)) {
> - printk(KERN_WARNING
> -"SELinux: failure in sel_netif_sid_slow(),"
> -" unable to determine network interface label (%d)\n",
> -ifindex);
> - kfree(new);
> - }
> return ret;
> }
>
> diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
> index da923f8..b752bd2 100644
> --- a/security/selinux/netnode.c
> +++ b/security/selinux/netnode.c
> @@ -199,7 +199,7 @@ static void sel_netnode_insert(struct sel_netnode *node)
> */
> static int sel_netnode_sid_slow(void *addr, u16 family, u32 *sid)
> {
> - int ret = -ENOMEM;
> + int ret;
> struct sel_netnode *node;
> struct sel_netnode *new = NULL;
>
> @@ -210,39 +210,42 @@ static int sel_netnode_sid_slow(void *addr, u16 family,
> u32 *sid)
> spin_unlock_bh(&sel_netnode_lock);
> return 0;
> }
> - new = kzalloc(sizeof(*new), GFP_ATOMIC);
> - if (new == NULL)
> - goto out;
> switch (family) {
> case PF_INET:
> ret = security_node_sid(PF_INET,
> addr, sizeof(struct in_addr), sid);
> - new->nsec.addr.ipv4 = *(__be32 *)addr;
> break;
> case PF_INET6:
> ret = security_node_sid(PF_INET6,
> addr, sizeof(struct in6_addr), sid);
> - new->nsec.addr.ipv6 = *(struct in6_addr *)addr;
> break;
> default:
> BUG();
> ret = -EINVAL;
> }
> - if (ret != 0)
> + if (ret != 0) {
> + printk(KERN_WARNING
> +"SELinux: failure in sel_netnode_sid_slow(),"
> +" unable to determine network node label\n");
> goto out;
> -
> + }
> + new = kzalloc(sizeof(*new), GFP_ATOMIC);
> + if (new == NULL)
> + goto out;
> + switch (family) {
> + case PF_INET:
> + new->nsec.addr.ipv4
Re: [RFC PATCH] selinux: always return a value from the netport/netnode/netif caches
On Wednesday, April 13, 2016 10:30:26 PM Daniel Jurgens wrote:
> On 4/13/2016 4:43 PM, Paul Moore wrote:
> > From: Paul Moore
> >
> > Even if we are under memory pressure and can't allocate a new cache
> > node we can still return the port/node/iface value we looked up from
> > the policy.
> >
> > Reported-by: Greg
> > Signed-off-by: Paul Moore
> > ---
> >
> > security/selinux/netif.c | 35 +--
> > security/selinux/netnode.c | 31 +--
> > security/selinux/netport.c | 19 ---
> > 3 files changed, 38 insertions(+), 47 deletions(-)
> >
> > diff --git a/security/selinux/netif.c b/security/selinux/netif.c
> > index e607b44..5c3bfa4 100644
> > --- a/security/selinux/netif.c
> > +++ b/security/selinux/netif.c
> > @@ -91,18 +91,16 @@ static inline struct sel_netif *sel_netif_find(const
> > struct net *ns,>
> > * zero on success, negative values on failure.
> > *
> > */
> >
> > -static int sel_netif_insert(struct sel_netif *netif)
> > +static void sel_netif_insert(struct sel_netif *netif)
> >
> > {
> >
> > int idx;
> >
> > if (sel_netif_total >= SEL_NETIF_HASH_MAX)
> >
> > - return -ENOSPC;
> > + return;
> >
> > idx = sel_netif_hashfn(netif->nsec.ns, netif->nsec.ifindex);
> > list_add_rcu(&netif->list, &sel_netif_hash[idx]);
> > sel_netif_total++;
> >
> > -
> > - return 0;
> >
> > }
> >
> > /**
> >
> > @@ -135,7 +133,7 @@ static void sel_netif_destroy(struct sel_netif *netif)
> >
> > */
> >
> > static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid)
> > {
> >
> > - int ret;
> > + int ret = 0;
> >
> > struct sel_netif *netif;
> > struct sel_netif *new = NULL;
> > struct net_device *dev;
> >
> > @@ -155,34 +153,27 @@ static int sel_netif_sid_slow(struct net *ns, int
> > ifindex, u32 *sid)>
> > netif = sel_netif_find(ns, ifindex);
>
> I know this is out of context for this patch, but isn't this find
> redundant? It was already checked in sel_netif_sid.
The first time we do the cache lookup it is only with the RCU read lock held,
we need to do another lookup once we are holding the spinlock.
--
paul moore
security @ redhat
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [RFC PATCH] selinux: always return a value from the netport/netnode/netif caches
On 4/13/2016 4:43 PM, Paul Moore wrote:
> From: Paul Moore
>
> Even if we are under memory pressure and can't allocate a new cache
> node we can still return the port/node/iface value we looked up from
> the policy.
>
> Reported-by: Greg
> Signed-off-by: Paul Moore
> ---
> security/selinux/netif.c | 35 +--
> security/selinux/netnode.c | 31 +--
> security/selinux/netport.c | 19 ---
> 3 files changed, 38 insertions(+), 47 deletions(-)
>
> diff --git a/security/selinux/netif.c b/security/selinux/netif.c
> index e607b44..5c3bfa4 100644
> --- a/security/selinux/netif.c
> +++ b/security/selinux/netif.c
> @@ -91,18 +91,16 @@ static inline struct sel_netif *sel_netif_find(const
> struct net *ns,
> * zero on success, negative values on failure.
> *
> */
> -static int sel_netif_insert(struct sel_netif *netif)
> +static void sel_netif_insert(struct sel_netif *netif)
> {
> int idx;
>
> if (sel_netif_total >= SEL_NETIF_HASH_MAX)
> - return -ENOSPC;
> + return;
>
> idx = sel_netif_hashfn(netif->nsec.ns, netif->nsec.ifindex);
> list_add_rcu(&netif->list, &sel_netif_hash[idx]);
> sel_netif_total++;
> -
> - return 0;
> }
>
> /**
> @@ -135,7 +133,7 @@ static void sel_netif_destroy(struct sel_netif *netif)
> */
> static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid)
> {
> - int ret;
> + int ret = 0;
> struct sel_netif *netif;
> struct sel_netif *new = NULL;
> struct net_device *dev;
> @@ -155,34 +153,27 @@ static int sel_netif_sid_slow(struct net *ns, int
> ifindex, u32 *sid)
> netif = sel_netif_find(ns, ifindex);
I know this is out of context for this patch, but isn't this find
redundant? It was already checked in sel_netif_sid.
> if (netif != NULL) {
> *sid = netif->nsec.sid;
> - ret = 0;
> goto out;
> }
> - new = kzalloc(sizeof(*new), GFP_ATOMIC);
> - if (new == NULL) {
> - ret = -ENOMEM;
> + ret = security_netif_sid(dev->name, sid);
> + if (ret != 0) {
> + printk(KERN_WARNING
> +"SELinux: failure in sel_netif_sid_slow(),"
> +" unable to determine network interface label (%d)\n",
> +ifindex);
> goto out;
> }
> - ret = security_netif_sid(dev->name, &new->nsec.sid);
> - if (ret != 0)
> + new = kzalloc(sizeof(*new), GFP_ATOMIC);
> + if (new == NULL)
> goto out;
> new->nsec.ns = ns;
> new->nsec.ifindex = ifindex;
> - ret = sel_netif_insert(new);
> - if (ret != 0)
> - goto out;
> - *sid = new->nsec.sid;
> + new->nsec.sid = *sid;
> + sel_netif_insert(new);
>
> out:
> spin_unlock_bh(&sel_netif_lock);
> dev_put(dev);
> - if (unlikely(ret)) {
> - printk(KERN_WARNING
> -"SELinux: failure in sel_netif_sid_slow(),"
> -" unable to determine network interface label (%d)\n",
> -ifindex);
> - kfree(new);
> - }
> return ret;
> }
>
> diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
> index da923f8..b752bd2 100644
> --- a/security/selinux/netnode.c
> +++ b/security/selinux/netnode.c
> @@ -199,7 +199,7 @@ static void sel_netnode_insert(struct sel_netnode *node)
> */
> static int sel_netnode_sid_slow(void *addr, u16 family, u32 *sid)
> {
> - int ret = -ENOMEM;
> + int ret;
> struct sel_netnode *node;
> struct sel_netnode *new = NULL;
>
> @@ -210,39 +210,42 @@ static int sel_netnode_sid_slow(void *addr, u16 family,
> u32 *sid)
> spin_unlock_bh(&sel_netnode_lock);
> return 0;
> }
> - new = kzalloc(sizeof(*new), GFP_ATOMIC);
> - if (new == NULL)
> - goto out;
> switch (family) {
> case PF_INET:
> ret = security_node_sid(PF_INET,
> addr, sizeof(struct in_addr), sid);
> - new->nsec.addr.ipv4 = *(__be32 *)addr;
> break;
> case PF_INET6:
> ret = security_node_sid(PF_INET6,
> addr, sizeof(struct in6_addr), sid);
> - new->nsec.addr.ipv6 = *(struct in6_addr *)addr;
> break;
> default:
> BUG();
> ret = -EINVAL;
> }
> - if (ret != 0)
> + if (ret != 0) {
> + printk(KERN_WARNING
> +"SELinux: failure in sel_netnode_sid_slow(),"
> +" unable to determine network node label\n");
> goto out;
> -
> + }
> + new = kzalloc(sizeof(*new), GFP_ATOMIC);
> + if (new == NULL)
> + goto out;
> + switch (family) {
> + case PF_INET:
> + new->nsec.addr.ipv4 = *(__be32 *)addr;
>
[RFC PATCH] selinux: always return a value from the netport/netnode/netif caches
From: Paul Moore
Even if we are under memory pressure and can't allocate a new cache
node we can still return the port/node/iface value we looked up from
the policy.
Reported-by: Greg
Signed-off-by: Paul Moore
---
security/selinux/netif.c | 35 +--
security/selinux/netnode.c | 31 +--
security/selinux/netport.c | 19 ---
3 files changed, 38 insertions(+), 47 deletions(-)
diff --git a/security/selinux/netif.c b/security/selinux/netif.c
index e607b44..5c3bfa4 100644
--- a/security/selinux/netif.c
+++ b/security/selinux/netif.c
@@ -91,18 +91,16 @@ static inline struct sel_netif *sel_netif_find(const struct
net *ns,
* zero on success, negative values on failure.
*
*/
-static int sel_netif_insert(struct sel_netif *netif)
+static void sel_netif_insert(struct sel_netif *netif)
{
int idx;
if (sel_netif_total >= SEL_NETIF_HASH_MAX)
- return -ENOSPC;
+ return;
idx = sel_netif_hashfn(netif->nsec.ns, netif->nsec.ifindex);
list_add_rcu(&netif->list, &sel_netif_hash[idx]);
sel_netif_total++;
-
- return 0;
}
/**
@@ -135,7 +133,7 @@ static void sel_netif_destroy(struct sel_netif *netif)
*/
static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid)
{
- int ret;
+ int ret = 0;
struct sel_netif *netif;
struct sel_netif *new = NULL;
struct net_device *dev;
@@ -155,34 +153,27 @@ static int sel_netif_sid_slow(struct net *ns, int
ifindex, u32 *sid)
netif = sel_netif_find(ns, ifindex);
if (netif != NULL) {
*sid = netif->nsec.sid;
- ret = 0;
goto out;
}
- new = kzalloc(sizeof(*new), GFP_ATOMIC);
- if (new == NULL) {
- ret = -ENOMEM;
+ ret = security_netif_sid(dev->name, sid);
+ if (ret != 0) {
+ printk(KERN_WARNING
+ "SELinux: failure in sel_netif_sid_slow(),"
+ " unable to determine network interface label (%d)\n",
+ ifindex);
goto out;
}
- ret = security_netif_sid(dev->name, &new->nsec.sid);
- if (ret != 0)
+ new = kzalloc(sizeof(*new), GFP_ATOMIC);
+ if (new == NULL)
goto out;
new->nsec.ns = ns;
new->nsec.ifindex = ifindex;
- ret = sel_netif_insert(new);
- if (ret != 0)
- goto out;
- *sid = new->nsec.sid;
+ new->nsec.sid = *sid;
+ sel_netif_insert(new);
out:
spin_unlock_bh(&sel_netif_lock);
dev_put(dev);
- if (unlikely(ret)) {
- printk(KERN_WARNING
- "SELinux: failure in sel_netif_sid_slow(),"
- " unable to determine network interface label (%d)\n",
- ifindex);
- kfree(new);
- }
return ret;
}
diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
index da923f8..b752bd2 100644
--- a/security/selinux/netnode.c
+++ b/security/selinux/netnode.c
@@ -199,7 +199,7 @@ static void sel_netnode_insert(struct sel_netnode *node)
*/
static int sel_netnode_sid_slow(void *addr, u16 family, u32 *sid)
{
- int ret = -ENOMEM;
+ int ret;
struct sel_netnode *node;
struct sel_netnode *new = NULL;
@@ -210,39 +210,42 @@ static int sel_netnode_sid_slow(void *addr, u16 family,
u32 *sid)
spin_unlock_bh(&sel_netnode_lock);
return 0;
}
- new = kzalloc(sizeof(*new), GFP_ATOMIC);
- if (new == NULL)
- goto out;
switch (family) {
case PF_INET:
ret = security_node_sid(PF_INET,
addr, sizeof(struct in_addr), sid);
- new->nsec.addr.ipv4 = *(__be32 *)addr;
break;
case PF_INET6:
ret = security_node_sid(PF_INET6,
addr, sizeof(struct in6_addr), sid);
- new->nsec.addr.ipv6 = *(struct in6_addr *)addr;
break;
default:
BUG();
ret = -EINVAL;
}
- if (ret != 0)
+ if (ret != 0) {
+ printk(KERN_WARNING
+ "SELinux: failure in sel_netnode_sid_slow(),"
+ " unable to determine network node label\n");
goto out;
-
+ }
+ new = kzalloc(sizeof(*new), GFP_ATOMIC);
+ if (new == NULL)
+ goto out;
+ switch (family) {
+ case PF_INET:
+ new->nsec.addr.ipv4 = *(__be32 *)addr;
+ break;
+ case PF_INET6:
+ new->nsec.addr.ipv6 = *(struct in6_addr *)addr;
+ break;
+ }
new->nsec.family = family;
new->nsec.sid = *sid;
sel_netnode_insert(new);
out:
spin_unlock_b
