Re: [PATCH v6] LSM: Multiple concurrent LSMs

Re: [PATCH v6] LSM: Multiple concurrent LSMs

[Casey Schaufler]

On 11/5/2012 5:52 PM, Kees Cook wrote:
> On Mon, Nov 5, 2012 at 4:56 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Version 6 of the patch addresses:
>>
>> 1. The array based hook calling loops have been
>>    replaced by hook lists. The blobs remain array
>>    based.
> I wonder if the entire security structure could be made into an array
> instead of named fields. Then instead of having these repeated giant
> lists of field names, we could trivially do for (i = 0; i <
> LSM_MAX_FUNC; i++) to walk them all. Solving the function prototyping
> part is, however, UGLY, and we lose mis-match detection if an LSM
> author messes up a declaration. This probably isn't worth it,

Believe me, when I was knee deep in security.c I thought about it.
In the end I decided that any given hook might end up having a special
behavior of some sort and it would be better to spell each hook out
than to have the next person who comes through have to deal with
pulling code out of macros.

A thought that I had was to move some of the registration logic into
the LSMs. Instead of passing a security_operations vector the LSM would
call the lsm_enlist function with the appropriate list for each hook
the LSM supports. If the LSM wanted reset_security_ops it could
lsm_delist all of its hooks. There would still have to be a way to
get the ops->order value set.

I think the modularity in place today makes it easier for the
LSM system to control things and easier for the LSM writer to
make things work right. I would hesitate to change it at this
point.

>  but
> here's what I was thinking:
>
> enum lsm_function {
>     LSM_PTRACE_ACCESS_CHECK = 0,
> ...
>     LSM_AUDIT_RULE_MATCH,
>     LSM_MAX_FUNC
> };
>
> struct security_operations {
>     char name[SECURITY_NAME_MAX + 1];
>     int order;
>     void (*func)(void)[LSM_MAX_FUNC];
> }
>
> ...
>
>
> static int __init lsm_enlist_ops(struct security_operations *sop)
> {
>     enum lsm_function func;
>
>     for (func = 0; func < LSM_MAX_FUNC; func++)
>         if (lsm_enlist(sop, func, sop->[func]))
>             return 1;
> }
>
> ...
>
> static lsm_ops[LSM_MAX_FUNCS];
>
> int __init security_init(void)
> {
>     enum lsm_functions func;
>
>     for (func = 0; func < LSM_MAX_FUNCS; func++)
>         INIT_LIST_HEAD(lsm_ops[func]);
>
>     do_security_initcalls();
>
>     pr_info("Security Framework initialized\n");
>
>     return 0;
> }
>
> ...
>
> int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
> {
>     /* The function prototypes would live in the security_* callers... */
>     int (*func)(struct task_struct *child, unsigned int mode);
>     struct lsm_list *lp;
>     int rc = 0;
>     int thisrc;
>
>     if (list_empty(lsm_ops[LSM_PTRACE_ACCESS_CHECK])) {
>         func =  capability_ops.func[LSM_PTRACE_ACCESS_CHECK];
>         return func(child, mode);
>
>     list_for_each_entry(lp, lsm_ops[LSM_PTRACE_ACCESS_CHECK], list) {
>         func =  lp->ops->func[LSM_PTRACE_ACCESS_CHECK];
>         thisrc = func(child, mode);
>         if (thisrc)
>             rc = thisrc;
>     }
>     return rc;
> }

That's an awful lot of "stuff" just to hide a few lines of code
that the developer ought to be looking at anyway.

>
>> 4. The security_hook funtions have been un-macroed.
>>    This should make dealing with special cases
>>    easier at the cost of code bulk.
> It seems like there are enough calls doing the same thing that leaving
> the common cases as macros is fine. As Tetsuo pointed out, they'll all
> get optimized.

I could be persuaded to go back to macros for trivial cases I suppose,
but now that it's all out in the open I kind of like it this way.

>
> Other than these thoughts, I say ship it! :)

I'll wait until Stephen points out the obvious configuration
issue that I've missed, and to see who points out the fundamental
flaw in my list handling.

Seriously though, suggestions on the XFRM and secid hook issues
would be greatly appreciated. I'm not sure that audit is completely
robust, either. And the whole [gs]etprocattr processing is too
clever by half. I deserve a serious flame on that and hope someone
can provide it and the follow-on correct code.

>
> Acked-by: Kees Cook <keescook@chromium.org>

Thanks. I think I may be getting close.

>
> -Kees
>


--
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.

Re: [PATCH v6] LSM: Multiple concurrent LSMs

[Casey Schaufler]

On 11/6/2012 4:44 AM, Tetsuo Handa wrote:
> Overall, this version can eliminate overhead caused by unimplemented hooks.
> Then, I think we can afford allowing legal registration of trivial LKM-based
> LSM modules after the init process starts; LSM modules which wants to use
> lsm_blobs are limited to COMPOSER_MAX but LSM modules which do not need
> lsm_blobs are allowed to chain as many as the administrator wants.

So if an LSM includes no hooks that use blobs none would get
allocated. Yama, for example. There would have to be a separate
load order from blob slot number. I'm tempted to say it's too
rare a case to worry about, but we have the example of Yama
sitting there challenging that assertion.

I will consider this.

> Below are trivial checks.
>
>
> +struct lsm_list {
> +    struct list_head        list;
> +    struct security_operations    *ops;
> +};
>
> +static void lsm_delist(struct security_operations *ops, struct
> list_head *list)
> +{
> +#ifdef CONFIG_SECURITY_COMPOSER_DEBUG
> +    static int leaked;
> +#endif
> +    struct lsm_list *lp;
> +
> +    list_for_each_entry(lp, list, list) {
> +        if (lp->ops == ops) {
> +            list_del_init(&lp->list);
> +#ifdef CONFIG_SECURITY_COMPOSER_DEBUG
> +            leaked += sizeof(*lp);
> +            pr_warn("%s lsm %s leaking %d bytes\n", __func__,
> +                ops->name, leaked);
> +#endif
> +            return;
> +        }
> +    }
> +}
>
> sizeof(*lp) should be 32 rather than 12 on x86_32 unless CONFIG_SLOB=y .

So the reality is that I'd be happy to report the number of lsm_list
blobs rather than the size, if that would be more palatable. Or, not
report a quantity at all, just say "Leaked some bytes while delisting
an LSM" and be done. Do you feel strongly one way or the other? Does
anyone else care?


> Seems unsafe list deletion. I think we need to use _rcu version since somebody
> (kernel threads) may be walking on the list when lsm_delist_ops() is called by
> the init process.

Here's the question, and I don't know the answer. lsm_delist() gets called
under two conditions: an early memory allocation failure that will almost
certainly precede a fatal memory allocation error, or a call to
security_reset_ops(), which is strictly a one-time thing. Might it be
better to have a lock of some sort to deal with the unlikely case of a
call to lsm_delist than to use the _rcu versions? I honestly don't know
what the cost of using _rcu is, so it may not matter.

Of course, I'd be fine with a panic if the LSM list entry allocation
failed because it just should never happen. As far as security_reset_ops()
is concerned, I think we'd only have to sell one distribution on the
virtue of removing it. Maybe.

> Maybe we want noinline attribute, for inlining lsm_delist() into
> lsm_delist_ops() is a bloat. And so does noinline attribute to lsm_enlist().

I'm hesitant to try to second guess the compilers on this. If a
performance expert told me to do it, I would, but I'd hate to be too
clever.

> +extern struct security_operations *composer_ops[];
>
> No longer used variable.

Right you are.

>  void security_bprm_committed_creds(struct linux_binprm *bprm)
>  {
> -    security_ops->bprm_committed_creds(bprm);
> +    struct lsm_list *lp;
> +
> +    list_for_each_entry(lp, &lsm_bprm_committed_creds, list)
> +        lp->ops->bprm_committed_creds(bprm);
>  }
>
> Seems unsafe list traversal. I think we need to use _rcu version since somebody
> (kernel threads) may be walking on the list when lsm_delist_ops() is called by
> the init process.

As discussed above, I'm OK to switch to _rcu if the performance
doesn't matter. It would be unfortunate to add unnecessary overhead
for such an extremely special case.

> +            for (; successes >= 0; successes--)
> +                note[successes]->cred_free(cred);
>
> Seems wrong starting point.
>
> 	note[successes++] = lp->ops;
>
> wants to start from --successes, which can be simplified like
>
> 	while (successes)
> 		note[--successes]->cred_free(cred);

Yup. Will fix for the next version.


--
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.

Re: [PATCH v6] LSM: Multiple concurrent LSMs

[Casey Schaufler]

On 11/7/2012 5:15 AM, Tetsuo Handa wrote:
> Reordering the message for explanation...
>
> Casey Schaufler wrote:
>> On 11/6/2012 4:44 AM, Tetsuo Handa wrote:
>>> sizeof(*lp) should be 32 rather than 12 on x86_32 unless CONFIG_SLOB=y .
>> So the reality is that I'd be happy to report the number of lsm_list
>> blobs rather than the size, if that would be more palatable. Or, not
>> report a quantity at all, just say "Leaked some bytes while delisting
>> an LSM" and be done. Do you feel strongly one way or the other? Does
>> anyone else care?
> ksize(lp) returns exact size compared to sizeof(*lp).

That was easy to adopt.

> But another approach is
> to embed "struct list_head" into "struct security_operations", something like
>
> 	struct security_operations {
> 		char name[SECURITY_NAME_MAX + 1];
> 	
> 		int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
> 		struct list_head ptrace_access_check_list;
> 		int (*ptrace_traceme) (struct task_struct *parent);
> 		struct list_head ptrace_traceme_list;
> 		int (*capget) (struct task_struct *target,
> 			       kernel_cap_t *effective,
> 			       kernel_cap_t *inheritable, kernel_cap_t *permitted);
> 		struct list_head capget_list;
> 		(...snipped...)
> 	};
>
> so that we can avoid kmalloc()/kfree() upon registration/unregistration.

How does one go about adding the hooks to the appropriate lists?
I came up with one macro based method that made Eric's hair fall
out, and he never even saw it. I don't think embedding a list for
each hook into the security_operations structure gets anything 
other than bloat. Because of typing you would need a separate
enlist and delist function for each hook. Try it if you don't
believe me.


>>> Seems unsafe list deletion. I think we need to use _rcu version since somebody
>>> (kernel threads) may be walking on the list when lsm_delist_ops() is called by
>>> the init process.
>> Here's the question, and I don't know the answer. lsm_delist() gets called
>> under two conditions: an early memory allocation failure that will almost
>> certainly precede a fatal memory allocation error, or a call to
>> security_reset_ops(), which is strictly a one-time thing. Might it be
>> better to have a lock of some sort to deal with the unlikely case of a
>> call to lsm_delist than to use the _rcu versions? I honestly don't know
>> what the cost of using _rcu is, so it may not matter.
> Is list_for_each_entry() safe against concurrent list_del() without locks?
> If I understand correctly, it isn't safe to call list_for_each_entry() without
> locks if list elements can be removed by list_del().
>
> I considered singly linked list (rather than using doubly linked list with _rcu
> primitives) so that list traversal without locks becomes safe with regard to
> list addition/deletion. Concurrent list addition/deletion is not safe, but we
> can afford using locks only for protecting such case because list
> addition/deletion happens only upon registration/unregistration.

That would entail fighting with the list macro use purists.

>>> Maybe we want noinline attribute, for inlining lsm_delist() into
>>> lsm_delist_ops() is a bloat. And so does noinline attribute to lsm_enlist().
>> I'm hesitant to try to second guess the compilers on this. If a
>> performance expert told me to do it, I would, but I'd hate to be too
>> clever.
> Thinking from how frequently lsm_enlist()/lsm_delist() is called, I think
> it does not worth inlining lsm_enlist()/lsm_delist() calls.

I agree, I also don't think is worth specifying noinline.
 

>>> Overall, this version can eliminate overhead caused by unimplemented hooks.
>>> Then, I think we can afford allowing legal registration of trivial LKM-based
>>> LSM modules after the init process starts; LSM modules which wants to use
>>> lsm_blobs are limited to COMPOSER_MAX but LSM modules which do not need
>>> lsm_blobs are allowed to chain as many as the administrator wants.
>> So if an LSM includes no hooks that use blobs none would get
>> allocated. Yama, for example. There would have to be a separate
>> load order from blob slot number. I'm tempted to say it's too
>> rare a case to worry about, but we have the example of Yama
>> sitting there challenging that assertion.
>>
>> I will consider this.
> Thank you.
> Does adding 'bool use_blobs' field to "struct security_operations" help?

Only if LSM writers are trustworthy and careful.
I expect that a check at registration on the hooks that use blobs
would be sufficient. I will check all the inode, file, cred and such
hooks and if any are called the LSM gets a slot.
 

>
> There are three reasons I want to allow legal registration of LKM-based LSM
> modules after the init process starts.
>
> First is that many distributions nowadays enable multiple LSM modules. Since
> LSM modules cannot be a LKM, all possible LSM modules have to be built-in,
> bloating the vmlinuz file. If LSM modules are allowed to be a LKM, distributors
> can reduce the size of vmlinuz (making the kernel load time shorter). TOMOYO is
> sitting here waiting for legal registration of LKM-based LSM modules.
>
> Second is that several distributions (e.g. Fedora and RHEL) enable only one LSM
> module (e.g. SELinux). On such distributions, users who want to use other LSM
> modules at their own risk have to replace the kernel package; this limitation
> is a strong psychological barrier and maintenance burden for such users. I'm
> using AKARI (which is a LKM-based TOMOYO) for troubleshooting RHEL systems
> because replacing the kernel package (in order to use TOMOYO) is unlikely
> acceptable for RHEL users.
>
> Third is that LSM is not the tool for thought control. LSM should be the tool
> for minimizing the burden of all subsystems' maintainers.
> I hope LSM folks have learned that "not everybody can configure SELinux" and
> "even AppArmor is not configurable for somebody"
> ( http://events.linuxfoundation.org/images/stories/pdf/lcna_co2012_handa.pdf )
> and "Yama is sufficient for some users" and other (not yet in-tree) modules may
> be sufficient for some other users
> ( http://events.linuxfoundation.org/images/stories/pdf/lcna_co2012_henderson.pdf ).
>
> For accelerating the development of custom LSM module which users really want,
> LSM interface should be opened to allow LKM-based LSM modules.
>


--
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.

Re: [PATCH v6] LSM: Multiple concurrent LSMs

[Eric Paris]

On Thu, Nov 8, 2012 at 8:28 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:

> How does one go about adding the hooks to the appropriate lists?
> I came up with one macro based method that made Eric's hair fall
> out, and he never even saw it.

This is your fault?  You jerk!  I want my hair back!

--
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.