[PATCH] selinux: init target class when add avc callback

[PATCH] selinux: init target class when add avc callback

[Wanlong Gao]

Target security class should be initialized when add avc callback.
Although tclass is userless in callbacks now, but it may be used
in the future .

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 security/selinux/avc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index dca1c22..27495e6 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
 	c->events = events;
 	c->ssid = ssid;
 	c->tsid = tsid;
+	c->tclass = tclass;
 	c->perms = perms;
 	c->next = avc_callbacks;
 	avc_callbacks = c;
-- 
1.7.9

Re: [PATCH] selinux: init target class when add avc callback

[Wanlong Gao]

On 02/05/2012 09:53 AM, Wanlong Gao wrote:

> Target security class should be initialized when add avc callback.
> Although tclass is userless in callbacks now, but it may be used
> in the future .
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  security/selinux/avc.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index dca1c22..27495e6 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
>  	c->events = events;
>  	c->ssid = ssid;
>  	c->tsid = tsid;
> +	c->tclass = tclass;
>  	c->perms = perms;
>  	c->next = avc_callbacks;
>  	avc_callbacks = c;

Re: [PATCH] selinux: init target class when add avc callback

[Andrew Morton]

On Wed, 07 Mar 2012 07:59:30 +0800
Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:

> On 02/05/2012 09:53 AM, Wanlong Gao wrote:
> 
> > Target security class should be initialized when add avc callback.
> > Although tclass is userless in callbacks now, but it may be used
> > in the future .
> > 
> > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > ---
> >  security/selinux/avc.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index dca1c22..27495e6 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
> >  	c->events = events;
> >  	c->ssid = ssid;
> >  	c->tsid = tsid;
> > +	c->tclass = tclass;
> >  	c->perms = perms;
> >  	c->next = avc_callbacks;
> >  	avc_callbacks = c;

Perhaps James can take a look at this?

avc_add_callback() looks a bit odd.  It uses GFP_ATOMIC, but that is
unnecessary because avc_add_callback() is only ever called from
module_init() code.  And if it isn't only ever called from
module_init() code then it needs some locking for that list.

Re: [PATCH] selinux: init target class when add avc callback

[Eric Paris]

On Tue, 2012-03-06 at 16:15 -0800, Andrew Morton wrote:
> On Wed, 07 Mar 2012 07:59:30 +0800
> Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> 
> > On 02/05/2012 09:53 AM, Wanlong Gao wrote:
> > 
> > > Target security class should be initialized when add avc callback.
> > > Although tclass is userless in callbacks now, but it may be used
> > > in the future .
> > > 
> > > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > > ---
> > >  security/selinux/avc.c |    1 +
> > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > > index dca1c22..27495e6 100644
> > > --- a/security/selinux/avc.c
> > > +++ b/security/selinux/avc.c
> > > @@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
> > >  	c->events = events;
> > >  	c->ssid = ssid;
> > >  	c->tsid = tsid;
> > > +	c->tclass = tclass;
> > >  	c->perms = perms;
> > >  	c->next = avc_callbacks;
> > >  	avc_callbacks = c;
> 
> Perhaps James can take a look at this?
> 
> avc_add_callback() looks a bit odd.  It uses GFP_ATOMIC, but that is
> unnecessary because avc_add_callback() is only ever called from
> module_init() code.  And if it isn't only ever called from
> module_init() code then it needs some locking for that list.

I'm a bad maintainer.  I should have done something with this patch.
Adding sds, the only other person who ever actually maintains this code,
to the thread.

__initcall() functions aren't serialized?  I guess that would be bad and
we would need a lock.  I wonder if there are other places I assumed
__initcall() would be serialized (note that all of these call sites are
built in and not modules if that makes a difference)

I'll probably just rip all of that ssid, tsid, tclass, perms, stuff out.
If all these years noone uses callbacks for anything other than reset
why do we have it at all.  Probably more simplification we can do around
avc_update_node() too...

Stephen, thoughts on ripping stuff out?

-Eric

Re: [PATCH] selinux: init target class when add avc callback

[Wanlong Gao]

On 03/07/2012 08:41 AM, Eric Paris wrote:

> On Tue, 2012-03-06 at 16:15 -0800, Andrew Morton wrote:
>> On Wed, 07 Mar 2012 07:59:30 +0800
>> Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
>>
>>> On 02/05/2012 09:53 AM, Wanlong Gao wrote:
>>>
>>>> Target security class should be initialized when add avc callback.
>>>> Although tclass is userless in callbacks now, but it may be used
>>>> in the future .
>>>>
>>>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>>> ---
>>>>  security/selinux/avc.c |    1 +
>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
>>>> index dca1c22..27495e6 100644
>>>> --- a/security/selinux/avc.c
>>>> +++ b/security/selinux/avc.c
>>>> @@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
>>>>  	c->events = events;
>>>>  	c->ssid = ssid;
>>>>  	c->tsid = tsid;
>>>> +	c->tclass = tclass;
>>>>  	c->perms = perms;
>>>>  	c->next = avc_callbacks;
>>>>  	avc_callbacks = c;
>>
>> Perhaps James can take a look at this?
>>
>> avc_add_callback() looks a bit odd.  It uses GFP_ATOMIC, but that is
>> unnecessary because avc_add_callback() is only ever called from
>> module_init() code.  And if it isn't only ever called from
>> module_init() code then it needs some locking for that list.
> 
> I'm a bad maintainer.  I should have done something with this patch.
> Adding sds, the only other person who ever actually maintains this code,
> to the thread.
> 
> __initcall() functions aren't serialized?  I guess that would be bad and
> we would need a lock.  I wonder if there are other places I assumed
> __initcall() would be serialized (note that all of these call sites are
> built in and not modules if that makes a difference)
> 
> I'll probably just rip all of that ssid, tsid, tclass, perms, stuff out.
> If all these years noone uses callbacks for anything other than reset
> why do we have it at all.  Probably more simplification we can do around
> avc_update_node() too...


Agree, seems that no one will use callbacks other than reset.

-Gao

> 
> Stephen, thoughts on ripping stuff out?
> 
> -Eric
> 
> 

Re: [PATCH] selinux: init target class when add avc callback

[Andrew Morton]

On Tue, 06 Mar 2012 19:41:33 -0500
Eric Paris <eparis@parisplace.org> wrote:

> __initcall() functions aren't serialized?  I guess that would be bad and
> we would need a lock.  I wonder if there are other places I assumed
> __initcall() would be serialized (note that all of these call sites are
> built in and not modules if that makes a difference)

There's plenty of code in the kernel which assumes that initcalls
are singly-threaded.  And init/main.c:do_initcalls() is very
singly-threaded!  It's less clear when the initcall is executed by
modprobe, but presumably there's something in the module code which
prevents concurrent execution of module_init() functions.

So I think the list management code is acceptable, as long as we ensure
that the function is only ever called from initcall functions.  We can
add a comment, but a neat way of ensuring this is to mark the function
__init.  This saves memory and will cause a build-time warning if we
screw up.

My point was that given that this function is only ever called from
initcalls, that weak GFP_ATOMIC could/should be replaced with
GFP_KERNEL.

Re: [PATCH] selinux: init target class when add avc callback

[Stephen Smalley]

On Tue, 2012-03-06 at 19:41 -0500, Eric Paris wrote:
> On Tue, 2012-03-06 at 16:15 -0800, Andrew Morton wrote:
> > On Wed, 07 Mar 2012 07:59:30 +0800
> > Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> > 
> > > On 02/05/2012 09:53 AM, Wanlong Gao wrote:
> > > 
> > > > Target security class should be initialized when add avc callback.
> > > > Although tclass is userless in callbacks now, but it may be used
> > > > in the future .
> > > > 
> > > > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > > > ---
> > > >  security/selinux/avc.c |    1 +
> > > >  1 files changed, 1 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > > > index dca1c22..27495e6 100644
> > > > --- a/security/selinux/avc.c
> > > > +++ b/security/selinux/avc.c
> > > > @@ -576,6 +576,7 @@ int avc_add_callback(int (*callback)(u32 event, u32 ssid, u32 tsid,
> > > >  	c->events = events;
> > > >  	c->ssid = ssid;
> > > >  	c->tsid = tsid;
> > > > +	c->tclass = tclass;
> > > >  	c->perms = perms;
> > > >  	c->next = avc_callbacks;
> > > >  	avc_callbacks = c;
> > 
> > Perhaps James can take a look at this?
> > 
> > avc_add_callback() looks a bit odd.  It uses GFP_ATOMIC, but that is
> > unnecessary because avc_add_callback() is only ever called from
> > module_init() code.  And if it isn't only ever called from
> > module_init() code then it needs some locking for that list.
> 
> I'm a bad maintainer.  I should have done something with this patch.
> Adding sds, the only other person who ever actually maintains this code,
> to the thread.
> 
> __initcall() functions aren't serialized?  I guess that would be bad and
> we would need a lock.  I wonder if there are other places I assumed
> __initcall() would be serialized (note that all of these call sites are
> built in and not modules if that makes a difference)
> 
> I'll probably just rip all of that ssid, tsid, tclass, perms, stuff out.
> If all these years noone uses callbacks for anything other than reset
> why do we have it at all.  Probably more simplification we can do around
> avc_update_node() too...
> 
> Stephen, thoughts on ripping stuff out?

Yes, you should be able to replace GFP_ATOMIC with GFP_KERNEL there and
rip out the other callback fields/arguments as they are presently
unused.  Legacy of the original Flask code, where there were other
avc_ss_* interfaces for revoke and friends.

-- 
Stephen Smalley
National Security Agency