Hi Tony,
On Sun, Jul 03, 2005 at 03:51:52PM -0700, Tony Jones wrote:
> On Sun, Jul 03, 2005 at 05:43:33PM +0200, Kurt Garloff wrote:
>
> > Note that we could think of getting rid of dummy; however, it's
> > still used as fallback for stubs that are not implemented by an
> > LSM. I did not want to change this with this patch set, though
> > I'd like to see it done if everyone agrees it's a good idea.
>
> I think this needs to happen (dummy goes away or it's contents are replaced
> by cap routines as appropriate).
I agree, but it's not trivial, as we need to review existing LSMs:
For whatever security_ops function that they don't implement and
where dummy and capability differ, we'll need to fix up.
Before someone goes there, we should agree that it's a good idea.
Currently, capability has only a small subset of functions implemented
and still gets the other ones patched in from dummy through verify()/
security_fixup_ops().
> Currently in security_init() verify(&dummy_security_ops) calling
> security_fixup_ops is a no-op in terms of modifying dummy. In your patch as
> you suggest verify(&capability_security_ops) now patches routines from dummy
> into this default capability_security_ops. The code is already too baroque,
> no point making it more complex.
The alternative would be making capability_security_ops complete.
The patching could be avoided completely by using something like
this in security.h:
if (security_ops->OPERATION == NULL)
return cap_OPERTAION(x,y,z)
Or, even better, after patches 2a, 2b, 3 have been applied:
# define COND_SECURITY(seop, def) \
(security_opt->seop == NULL) || \
security_ops == &capability_security_ops)? \
def: security_ops->seop
The latter has the very nice side effect that we DON'T need to code
all the {return 0;}; and {}; functions into capability.c, but rather
use the return values we know already, because they are part of
security.h anyway for the !defined(CONFIG_SECURITY) case.
The more I think about it, the better I like that approach.
It will also be a nice performance improvement again:
Instead of unconditionally calling a function pointer (which
includes passing all the argument setup, cleanup, ...), we just
have one branch (that will be correctly predicted after the second
occurence) and know the return value immediately. Nice.
Allowing such things was one of my motivations of cleaning up
security.h.
But before, we really need to make sure all LSMs work with
security_fixup_ops() patching in capability ops and not dummy ops.
> I think the necessary functions need to be present in this new base structure
> and security_fixup_ops patches from it into whatever new ops a caller passes,
> dummy goes away. As a bonus, at this point your capability_ops struct in
> capability.c can really be different :-)
I don't consider this a bonus. Loading capability should not change
the behaviour IMVHO.[*]
The fact that the capability LSM may still be compiled, despite being
pointless (it's the default now), is that you may load it as secondary
LSM. I clarified this in the comment.
[*] Well, we could consider enhancing capability in the future and
leaving the old behaviour default for both !defined(CONFIG_SECURITY)
and the no-LSM-loaded case and have slightly enhanced semantics by
loading capability. I would prefer renaming the LSM then however.
> --- linux-2.6.10.orig/security/commoncap.c
> +++ linux-2.6.10/security/commoncap.c
> [snip]
> > +EXPORT_SYMBOL(capability_security_ops);
> > +/* Note: If the capability security module is loaded, we do NOT register
> > + * the capability_security_ops but a second structure that has the
> > + * identical entries. The reason is that this way,
> > + * - we could stack on top of capability if it was stackable
> > + * - a loaded capability module will prevent others to register, which
> > + * is the previous behaviour; if capabilities are used as default (not
> > + * because the module has been loaded), we allow the replacement.
> > + */
> > +#endif
>
> The intent here is to make it past the
> if (security_ops != &capability_security_ops)
> check in security.c::register_security so that it is possible to actually
> register capability (capability_ops) subsequently as a module should you
> so desire, no?
Correct.
> The above description doesn't impart that. Plus why would you want to stack
> capability on top of this base capability, even if it it supported stacking?
I could have disabled the creation of the capability LSM completely, but
who knows whether or not capability could be useful as secondary LSM
(or as part of some stack using stacker)? I can't exclude that ...
I have improved the comment, please review attached patch.
> --- linux-2.6.10.orig/security/capability.c
> +++ linux-2.6.10/security/capability.c
>
> +/* Note: If the capability security module is loaded, we do NOT register
> + * the capability_security_ops but a second structure capability_ops
> + * that has the identical entries. The reasons:
> + * - we could stack on top of capability if it was stackable
> + * - a loaded capability module will prevent others to register, which
> + * is the previous behaviour; if capabilities are used as default (not
> + * because the module has been loaded), we allow the replacement.
>
> Ditto for this comment.
Yep, changed as well.
>
> > static int __init capability_init (void)
> > {
> > + memcpy(&capability_ops, &capability_security_ops, sizeof(capability_ops));
> > if (capability_disable) {
> > printk(KERN_INFO "Capabilities disabled at initialization\n");
> > return 0;
> > }
>
> No point doing the memcpy if capability_disable is true.
We could move it below the check.
I've done so, please review attached patch.
(Sidenote: Patch 3 will have a trivial reject after the changes here.)
Thanks a lot for your comments!
--
Kurt Garloff, Director SUSE Labs, Novell Inc.
This archive was generated by hypermail 2.1.3 : Tue Jul 05 2005 - 05:49:41 PDT