| Message ID | 146058346288.11989.9543589385643222847.stgit@localhost (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On 4/13/2016 4:43 PM, Paul Moore wrote: > From: Paul Moore <paul@paul-moore.com> > > 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 <gkubok@gmail.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > 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; > + 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_bh(&sel_netnode_lock); > - if (unlikely(ret)) { > - printk(KERN_WARNING > - "SELinux: failure in sel_netnode_sid_slow()," > - " unable to determine network node label\n"); > - kfree(new); > - } > return ret; > } > > diff --git a/security/selinux/netport.c b/security/selinux/netport.c > index 3311cc3..189c293 100644 > --- a/security/selinux/netport.c > +++ b/security/selinux/netport.c > @@ -147,7 +147,7 @@ static void sel_netport_insert(struct sel_netport *port) > */ > static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > { > - int ret = -ENOMEM; > + int ret; > struct sel_netport *port; > struct sel_netport *new = NULL; > > @@ -158,13 +158,16 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > spin_unlock_bh(&sel_netport_lock); > return 0; > } > + ret = security_port_sid(protocol, pnum, sid); > + if (ret != 0) { > + printk(KERN_WARNING > + "SELinux: failure in sel_netport_sid_slow()," > + " unable to determine network port label\n"); > + goto out; > + } > new = kzalloc(sizeof(*new), GFP_ATOMIC); > if (new == NULL) > goto out; > - ret = security_port_sid(protocol, pnum, sid); > - if (ret != 0) > - goto out; > - > new->psec.port = pnum; > new->psec.protocol = protocol; > new->psec.sid = *sid; > @@ -172,12 +175,6 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > > out: > spin_unlock_bh(&sel_netport_lock); > - if (unlikely(ret)) { > - printk(KERN_WARNING > - "SELinux: failure in sel_netport_sid_slow()," > - " unable to determine network port label\n"); > - kfree(new); > - } > return ret; > } > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >
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 <paul@paul-moore.com> > > > > 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 <gkubok@gmail.com> > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > > > 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.
On 04/13/2016 05:37 PM, Paul Moore wrote: > From: Paul Moore <paul@paul-moore.com> > > 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 <gkubok@gmail.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > 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 = *(__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_bh(&sel_netnode_lock); > - if (unlikely(ret)) { > - printk(KERN_WARNING > - "SELinux: failure in sel_netnode_sid_slow()," > - " unable to determine network node label\n"); > - kfree(new); > - } > return ret; > } > > diff --git a/security/selinux/netport.c b/security/selinux/netport.c > index 3311cc3..189c293 100644 > --- a/security/selinux/netport.c > +++ b/security/selinux/netport.c > @@ -147,7 +147,7 @@ static void sel_netport_insert(struct sel_netport *port) > */ > static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > { > - int ret = -ENOMEM; > + int ret; > struct sel_netport *port; > struct sel_netport *new = NULL; > > @@ -158,13 +158,16 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > spin_unlock_bh(&sel_netport_lock); > return 0; > } > + ret = security_port_sid(protocol, pnum, sid); > + if (ret != 0) { > + printk(KERN_WARNING > + "SELinux: failure in sel_netport_sid_slow()," > + " unable to determine network port label\n"); > + goto out; > + } > new = kzalloc(sizeof(*new), GFP_ATOMIC); > if (new == NULL) > goto out; > - ret = security_port_sid(protocol, pnum, sid); > - if (ret != 0) > - goto out; > - > new->psec.port = pnum; > new->psec.protocol = protocol; > new->psec.sid = *sid; > @@ -172,12 +175,6 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) > > out: > spin_unlock_bh(&sel_netport_lock); > - if (unlikely(ret)) { > - printk(KERN_WARNING > - "SELinux: failure in sel_netport_sid_slow()," > - " unable to determine network port label\n"); > - kfree(new); > - } > return ret; > } > > > _______________________________________________ > Selinux mailing list > Selinux@tycho.nsa.gov > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov. > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov. >
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 <paul@paul-moore.com> > > > > 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 <gkubok@gmail.com> > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > --- > > > > 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.
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_bh(&sel_netnode_lock); - if (unlikely(ret)) { - printk(KERN_WARNING - "SELinux: failure in sel_netnode_sid_slow()," - " unable to determine network node label\n"); - kfree(new); - } return ret; } diff --git a/security/selinux/netport.c b/security/selinux/netport.c index 3311cc3..189c293 100644 --- a/security/selinux/netport.c +++ b/security/selinux/netport.c @@ -147,7 +147,7 @@ static void sel_netport_insert(struct sel_netport *port) */ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) { - int ret = -ENOMEM; + int ret; struct sel_netport *port; struct sel_netport *new = NULL; @@ -158,13 +158,16 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) spin_unlock_bh(&sel_netport_lock); return 0; } + ret = security_port_sid(protocol, pnum, sid); + if (ret != 0) { + printk(KERN_WARNING + "SELinux: failure in sel_netport_sid_slow()," + " unable to determine network port label\n"); + goto out; + } new = kzalloc(sizeof(*new), GFP_ATOMIC); if (new == NULL) goto out; - ret = security_port_sid(protocol, pnum, sid); - if (ret != 0) - goto out; - new->psec.port = pnum; new->psec.protocol = protocol; new->psec.sid = *sid; @@ -172,12 +175,6 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) out: spin_unlock_bh(&sel_netport_lock); - if (unlikely(ret)) { - printk(KERN_WARNING - "SELinux: failure in sel_netport_sid_slow()," - " unable to determine network port label\n"); - kfree(new); - } return ret; }