On Fri, 2008-03-07 at 15:36 -0500, Eric Paris wrote:
> On Fri, 2008-03-07 at 15:09 -0500, Stephen Smalley wrote:
> > On Fri, 2008-03-07 at 14:44 -0500, Eric Paris wrote:
>
> > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > > index 187964e..3eb5d69 100644
> > > --- a/security/selinux/avc.c
> > > +++ b/security/selinux/avc.c
> > > @@ -891,8 +891,15 @@ int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> > > denied = requested & ~(p_ae->avd.allowed);
> > >
> > > if (!requested || denied) {
> > > - if (selinux_enforcing || (flags & AVC_STRICT))
> > > + if (flags & AVC_STRICT)
> > > rc = -EACCES;
> > > + else if (selinux_enforcing)
> > > + if (unlikely(security_permissive_sid(ssid) &&
> > > + requested && node))
> > > + avc_update_node(AVC_CALLBACK_GRANT,requested,
> > > + ssid,tsid,tclass);
> > > + else
> > > + rc = -EACCES;
> > > else
> > > if (node)
> > > avc_update_node(AVC_CALLBACK_GRANT,requested,
> >
> > Can you unify the branches above so only one avc_update_node() call is
> > present?
>
> Yeah, I'm sure I can come up with something.
>
> else if (!selinux_enforcing ||
> (unlikely(security_permissive_sid(ssid)) && requested && node))
> avc_update_node(AVC_CALLBACK_GRANT.....
> else
> rc = -EACCES;
>
>
>
> > > -#define POLICYDB_VERSION_MAX POLICYDB_VERSION_POLCAP
> > > +#define POLICYDB_VERSION_MAX POLICYDB_VERSION_PERMISSIVE
>
> Will make a similar change in Kconfig
>
> > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > > index f374186..97b4453 100644
> > > --- a/security/selinux/ss/services.c
> > > +++ b/security/selinux/ss/services.c
> > > @@ -416,6 +416,21 @@ inval_class:
> > > return -EINVAL;
> > > }
> > >
> > > +/*
> > > + * Given a sid find if the type has the permissive flag set
> > > + */
> > > +int security_permissive_sid(u32 sid)
> > > +{
> > > + struct context *context;
> > > + u32 type;
> > > +
> > > + context = sidtab_search(&sidtab, sid);
> > > + BUG_ON(!context);
> > > +
> > > + type = context->type;
> > > + return ebitmap_get_bit(&policydb.permissive_map, type);
> >
> > This all needs to happen under policy rdlock - take it before looking up
> > in the sidtab and release after ebitmap_get_bit (so you have to save
> > result to temporary var and return that). Otherwise, it can get freed
> > under you by policy reload.
>
> Do I only need to worry about it freeing under me or should I also worry
> about atomicity from security_compute_av() ?
Well, that's a separate issue. Interesting to be sure, but separate.
> Do I really have any
> guarantee that sid is still in the sidtab here? My BUG_ON() might be
> bogus, right?
Not really. sidtab_search was changed long ago to remap undefined SIDs
to the unlabeled SID.
> Should I move the permissive bit check into
> security_compute_av() and carry about the bit in the avd_decision?
Yuck.
> Is
> doing an ebitmap_get too high of a penalty to pay for all access as
> opposed to this way which only does it for denials?
Not too heavy for compute_av (happens in constraint eval), but too heavy
for the AVC. But I'm not sure we need it.
> If we don't want
> the penalty on successful permission requests I can just drop the
> BUG_ON() and not worry about things changing under us....
--
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@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
