* Re: [PATCH 2/2] SELinux: check seqno when updating an avc_node
2009-02-12 19:50 ` [PATCH 2/2] SELinux: check seqno when updating an avc_node Eric Paris
@ 2009-02-13 14:07 ` Stephen Smalley
2009-02-13 15:06 ` Eric Paris
2009-02-13 20:19 ` Stephen Smalley
2009-02-13 22:44 ` James Morris
2 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2009-02-13 14:07 UTC (permalink / raw)
To: Eric Paris; +Cc: selinux, jmorris, paul.moore
On Thu, 2009-02-12 at 14:50 -0500, Eric Paris wrote:
> The avc update node callbacks do not check the seqno of the caller with the
> seqno of the node found. It is possible that a policy change could happen
> (although almost impossibly unlikely) in which a permissive or
> permissive_domain decision is not valid for the entry found. Simply pass
> and check that the seqno of the caller and the seqno of the node found
> match.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Changing enforcing/permissive mode doesn't change the seqno.
Also, the "caller" seqno in this case (p_ae->avd.seqno) is just the
policy seqno at the time the AVC entry was created, which may or may not
be the latest seqno. And we don't actually know what the seqno for the
security_permissive_sid() call. So I don't think this yields any
improvement.
NAK.
> ---
>
> security/selinux/avc.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index e5cda02..703aba1 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -747,13 +747,15 @@ static inline int avc_sidcmp(u32 x, u32 y)
> * @event : Updating event
> * @perms : Permission mask bits
> * @ssid,@tsid,@tclass : identifier of an AVC entry
> + * @seqno : sequence number when decision was made
> *
> * if a valid AVC entry doesn't exist,this function returns -ENOENT.
> * if kmalloc() called internal returns NULL, this function returns -ENOMEM.
> * otherwise, this function update the AVC entry. The original AVC-entry object
> * will release later by RCU.
> */
> -static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> +static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass,
> + u32 seqno)
> {
> int hvalue, rc = 0;
> unsigned long flag;
> @@ -772,7 +774,8 @@ static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> list_for_each_entry(pos, &avc_cache.slots[hvalue], list) {
> if (ssid == pos->ae.ssid &&
> tsid == pos->ae.tsid &&
> - tclass == pos->ae.tclass){
> + tclass == pos->ae.tclass &&
> + seqno == pos->ae.avd.seqno){
> orig = pos;
> break;
> }
> @@ -913,7 +916,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> rc = -EACCES;
> else if (!selinux_enforcing || security_permissive_sid(ssid))
> avc_update_node(AVC_CALLBACK_GRANT, requested, ssid,
> - tsid, tclass);
> + tsid, tclass, p_ae->avd.seqno);
> else
> rc = -EACCES;
> }
--
Stephen Smalley
National Security Agency
--
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] SELinux: check seqno when updating an avc_node
2009-02-13 14:07 ` Stephen Smalley
@ 2009-02-13 15:06 ` Eric Paris
2009-02-13 20:19 ` Stephen Smalley
0 siblings, 1 reply; 10+ messages in thread
From: Eric Paris @ 2009-02-13 15:06 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, jmorris, paul.moore
On Fri, 2009-02-13 at 09:07 -0500, Stephen Smalley wrote:
> On Thu, 2009-02-12 at 14:50 -0500, Eric Paris wrote:
> > The avc update node callbacks do not check the seqno of the caller with the
> > seqno of the node found. It is possible that a policy change could happen
> > (although almost impossibly unlikely) in which a permissive or
> > permissive_domain decision is not valid for the entry found. Simply pass
> > and check that the seqno of the caller and the seqno of the node found
> > match.
> >
> > Signed-off-by: Eric Paris <eparis@redhat.com>
>
> Changing enforcing/permissive mode doesn't change the seqno.
> Also, the "caller" seqno in this case (p_ae->avd.seqno) is just the
> policy seqno at the time the AVC entry was created, which may or may not
> be the latest seqno. And we don't actually know what the seqno for the
> security_permissive_sid() call. So I don't think this yields any
> improvement.
>
> NAK.
>
> > ---
> >
> > security/selinux/avc.c | 9 ++++++---
> > 1 files changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index e5cda02..703aba1 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -747,13 +747,15 @@ static inline int avc_sidcmp(u32 x, u32 y)
> > * @event : Updating event
> > * @perms : Permission mask bits
> > * @ssid,@tsid,@tclass : identifier of an AVC entry
> > + * @seqno : sequence number when decision was made
> > *
> > * if a valid AVC entry doesn't exist,this function returns -ENOENT.
> > * if kmalloc() called internal returns NULL, this function returns -ENOMEM.
> > * otherwise, this function update the AVC entry. The original AVC-entry object
> > * will release later by RCU.
> > */
> > -static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> > +static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass,
> > + u32 seqno)
> > {
> > int hvalue, rc = 0;
> > unsigned long flag;
> > @@ -772,7 +774,8 @@ static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> > list_for_each_entry(pos, &avc_cache.slots[hvalue], list) {
> > if (ssid == pos->ae.ssid &&
> > tsid == pos->ae.tsid &&
> > - tclass == pos->ae.tclass){
> > + tclass == pos->ae.tclass &&
> > + seqno == pos->ae.avd.seqno){
> > orig = pos;
> > break;
> > }
> > @@ -913,7 +916,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > rc = -EACCES;
> > else if (!selinux_enforcing || security_permissive_sid(ssid))
- Lets says in policy #1 ssid is permissive.
- Above we just entered this avd into the avc_cache.
- Lets say that right here we race with security_load_policy().
- That is going to inc the seqno and is going to delete our node. I
don't see anything sync'ing between these two thing. Certainly not RCU
lock.
- In policy #2 ssid is NOT permissive.
- Another process requests the same permission and adds the new node
with seqno=#2.
- We call avc_update_node and update the new node with the decision we
made from the old policy. Now the avc_cache has an entry that is wrong.
Now we know that the p_ae->avd.seqno has to be <= the current seqno
(thanks to all of that being done under the policy_rw lock.) If the
callback finds the old deleted node who cares if it gets updated, it's
on it's way out. If we find the new node with the higher seqno we just
do nothing.
It is possible that we instead raced before the permissive_sid call and
the decision is valid. Well, that's find, we are still going to return
allow for this operation and the avc_cache can just get updated the next
time.
Where am I missing the synchronization between a policy load and the
differences in what we would find from the original lookup and the
permissive sid lookup and the entry in the avc_cache?
-Eric
> > avc_update_node(AVC_CALLBACK_GRANT, requested, ssid,
> > - tsid, tclass);
> > + tsid, tclass, p_ae->avd.seqno);
> > else
> > rc = -EACCES;
> > }
--
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] SELinux: check seqno when updating an avc_node
2009-02-13 15:06 ` Eric Paris
@ 2009-02-13 20:19 ` Stephen Smalley
0 siblings, 0 replies; 10+ messages in thread
From: Stephen Smalley @ 2009-02-13 20:19 UTC (permalink / raw)
To: Eric Paris; +Cc: selinux, jmorris, paul.moore
On Fri, 2009-02-13 at 10:06 -0500, Eric Paris wrote:
> On Fri, 2009-02-13 at 09:07 -0500, Stephen Smalley wrote:
> > On Thu, 2009-02-12 at 14:50 -0500, Eric Paris wrote:
> > > The avc update node callbacks do not check the seqno of the caller with the
> > > seqno of the node found. It is possible that a policy change could happen
> > > (although almost impossibly unlikely) in which a permissive or
> > > permissive_domain decision is not valid for the entry found. Simply pass
> > > and check that the seqno of the caller and the seqno of the node found
> > > match.
> > >
> > > Signed-off-by: Eric Paris <eparis@redhat.com>
> >
> > Changing enforcing/permissive mode doesn't change the seqno.
> > Also, the "caller" seqno in this case (p_ae->avd.seqno) is just the
> > policy seqno at the time the AVC entry was created, which may or may not
> > be the latest seqno. And we don't actually know what the seqno for the
> > security_permissive_sid() call. So I don't think this yields any
> > improvement.
> >
> > NAK.
> >
> > > ---
> > >
> > > security/selinux/avc.c | 9 ++++++---
> > > 1 files changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > > index e5cda02..703aba1 100644
> > > --- a/security/selinux/avc.c
> > > +++ b/security/selinux/avc.c
> > > @@ -747,13 +747,15 @@ static inline int avc_sidcmp(u32 x, u32 y)
> > > * @event : Updating event
> > > * @perms : Permission mask bits
> > > * @ssid,@tsid,@tclass : identifier of an AVC entry
> > > + * @seqno : sequence number when decision was made
> > > *
> > > * if a valid AVC entry doesn't exist,this function returns -ENOENT.
> > > * if kmalloc() called internal returns NULL, this function returns -ENOMEM.
> > > * otherwise, this function update the AVC entry. The original AVC-entry object
> > > * will release later by RCU.
> > > */
> > > -static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> > > +static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass,
> > > + u32 seqno)
> > > {
> > > int hvalue, rc = 0;
> > > unsigned long flag;
> > > @@ -772,7 +774,8 @@ static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> > > list_for_each_entry(pos, &avc_cache.slots[hvalue], list) {
> > > if (ssid == pos->ae.ssid &&
> > > tsid == pos->ae.tsid &&
> > > - tclass == pos->ae.tclass){
> > > + tclass == pos->ae.tclass &&
> > > + seqno == pos->ae.avd.seqno){
> > > orig = pos;
> > > break;
> > > }
> > > @@ -913,7 +916,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > > rc = -EACCES;
> > > else if (!selinux_enforcing || security_permissive_sid(ssid))
>
> - Lets says in policy #1 ssid is permissive.
> - Above we just entered this avd into the avc_cache.
> - Lets say that right here we race with security_load_policy().
> - That is going to inc the seqno and is going to delete our node. I
> don't see anything sync'ing between these two thing. Certainly not RCU
> lock.
> - In policy #2 ssid is NOT permissive.
> - Another process requests the same permission and adds the new node
> with seqno=#2.
> - We call avc_update_node and update the new node with the decision we
> made from the old policy. Now the avc_cache has an entry that is wrong.
>
> Now we know that the p_ae->avd.seqno has to be <= the current seqno
> (thanks to all of that being done under the policy_rw lock.) If the
> callback finds the old deleted node who cares if it gets updated, it's
> on it's way out. If we find the new node with the higher seqno we just
> do nothing.
>
> It is possible that we instead raced before the permissive_sid call and
> the decision is valid. Well, that's find, we are still going to return
> allow for this operation and the avc_cache can just get updated the next
> time.
>
> Where am I missing the synchronization between a policy load and the
> differences in what we would find from the original lookup and the
> permissive sid lookup and the entry in the avc_cache?
Ok. Possible future improvements:
- deal with global permissive -> enforcing changes (no seqno change
there presently),
- provide permissive sid info as part of the avd or return the seqno
from security_permissive_sid
I'll ack the patch though...
--
Stephen Smalley
National Security Agency
--
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] SELinux: check seqno when updating an avc_node
2009-02-12 19:50 ` [PATCH 2/2] SELinux: check seqno when updating an avc_node Eric Paris
2009-02-13 14:07 ` Stephen Smalley
@ 2009-02-13 20:19 ` Stephen Smalley
2009-02-13 22:44 ` James Morris
2 siblings, 0 replies; 10+ messages in thread
From: Stephen Smalley @ 2009-02-13 20:19 UTC (permalink / raw)
To: Eric Paris; +Cc: selinux, jmorris, paul.moore
On Thu, 2009-02-12 at 14:50 -0500, Eric Paris wrote:
> The avc update node callbacks do not check the seqno of the caller with the
> seqno of the node found. It is possible that a policy change could happen
> (although almost impossibly unlikely) in which a permissive or
> permissive_domain decision is not valid for the entry found. Simply pass
> and check that the seqno of the caller and the seqno of the node found
> match.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>
> security/selinux/avc.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index e5cda02..703aba1 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -747,13 +747,15 @@ static inline int avc_sidcmp(u32 x, u32 y)
> * @event : Updating event
> * @perms : Permission mask bits
> * @ssid,@tsid,@tclass : identifier of an AVC entry
> + * @seqno : sequence number when decision was made
> *
> * if a valid AVC entry doesn't exist,this function returns -ENOENT.
> * if kmalloc() called internal returns NULL, this function returns -ENOMEM.
> * otherwise, this function update the AVC entry. The original AVC-entry object
> * will release later by RCU.
> */
> -static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> +static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass,
> + u32 seqno)
> {
> int hvalue, rc = 0;
> unsigned long flag;
> @@ -772,7 +774,8 @@ static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> list_for_each_entry(pos, &avc_cache.slots[hvalue], list) {
> if (ssid == pos->ae.ssid &&
> tsid == pos->ae.tsid &&
> - tclass == pos->ae.tclass){
> + tclass == pos->ae.tclass &&
> + seqno == pos->ae.avd.seqno){
> orig = pos;
> break;
> }
> @@ -913,7 +916,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> rc = -EACCES;
> else if (!selinux_enforcing || security_permissive_sid(ssid))
> avc_update_node(AVC_CALLBACK_GRANT, requested, ssid,
> - tsid, tclass);
> + tsid, tclass, p_ae->avd.seqno);
> else
> rc = -EACCES;
> }
>
>
> --
> 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.
--
Stephen Smalley
National Security Agency
--
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] SELinux: check seqno when updating an avc_node
2009-02-12 19:50 ` [PATCH 2/2] SELinux: check seqno when updating an avc_node Eric Paris
2009-02-13 14:07 ` Stephen Smalley
2009-02-13 20:19 ` Stephen Smalley
@ 2009-02-13 22:44 ` James Morris
2 siblings, 0 replies; 10+ messages in thread
From: James Morris @ 2009-02-13 22:44 UTC (permalink / raw)
To: Eric Paris; +Cc: selinux, sds, paul.moore
On Thu, 12 Feb 2009, Eric Paris wrote:
> The avc update node callbacks do not check the seqno of the caller with the
> seqno of the node found. It is possible that a policy change could happen
> (although almost impossibly unlikely) in which a permissive or
> permissive_domain decision is not valid for the entry found. Simply pass
> and check that the seqno of the caller and the seqno of the node found
> match.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Applied.
> ---
>
> security/selinux/avc.c | 9 ++++++---
> 1 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index e5cda02..703aba1 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -747,13 +747,15 @@ static inline int avc_sidcmp(u32 x, u32 y)
> * @event : Updating event
> * @perms : Permission mask bits
> * @ssid,@tsid,@tclass : identifier of an AVC entry
> + * @seqno : sequence number when decision was made
> *
> * if a valid AVC entry doesn't exist,this function returns -ENOENT.
> * if kmalloc() called internal returns NULL, this function returns -ENOMEM.
> * otherwise, this function update the AVC entry. The original AVC-entry object
> * will release later by RCU.
> */
> -static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> +static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass,
> + u32 seqno)
> {
> int hvalue, rc = 0;
> unsigned long flag;
> @@ -772,7 +774,8 @@ static int avc_update_node(u32 event, u32 perms, u32 ssid, u32 tsid, u16 tclass)
> list_for_each_entry(pos, &avc_cache.slots[hvalue], list) {
> if (ssid == pos->ae.ssid &&
> tsid == pos->ae.tsid &&
> - tclass == pos->ae.tclass){
> + tclass == pos->ae.tclass &&
> + seqno == pos->ae.avd.seqno){
> orig = pos;
> break;
> }
> @@ -913,7 +916,7 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> rc = -EACCES;
> else if (!selinux_enforcing || security_permissive_sid(ssid))
> avc_update_node(AVC_CALLBACK_GRANT, requested, ssid,
> - tsid, tclass);
> + tsid, tclass, p_ae->avd.seqno);
> else
> rc = -EACCES;
> }
>
--
James Morris
<jmorris@namei.org>
--
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.
^ permalink raw reply [flat|nested] 10+ messages in thread