[PATCH v4 00/19] LSM: Module stacking for SARA and Landlock

On 9/24/2018 8:01 AM, Stephen Smalley wrote:

On 09/23/2018 01:09 PM, Casey Schaufler wrote:

quoted

On 9/23/2018 8:59 AM, Tetsuo Handa wrote:

quoted

On 2018/09/23 11:43, Kees Cook wrote:

quoted

quoted

quoted

I'm excited about getting this landed!

Soon. Real soon. I hope. I would very much like for
someone from the SELinux camp to chime in, especially on
the selinux_is_enabled() removal.

Agreed.

This patchset from Casey lands before the patchset from Kees, doesn't it?

That is up for negotiation. We may end up combining them.

quoted

OK, a few comments (if I didn't overlook something).

?? lsm_early_cred()/lsm_early_task() are called from only __init functions.

True.

quoted

?? lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c .

Also true.

quoted

?? lsm_early_inode() should be avoided because it is not appropriate to
?? call panic() when lsm_early_inode() is called after __init phase.

You're correct. In fact, lsm_early_inode() isn't needed at all
until multiple inode using modules are supported.

quoted

?? Since all free hooks are called when one of init hooks failed, each
?? free hook needs to check whether init hook was called. An example is
?? inode_free_security() in security/selinux/hooks.c (but not addressed in
?? this patch).

I *think* that selinux_inode_free_security() is safe in this
case because the blob will be zeroed, hence isec->list will
be NULL.

That's not safe - look more closely at what list_empty_careful() tests, and then think about what happens when list_del_init() gets called on that isec->list.? selinux_inode_free_security() presumes that selinux_inode_alloc_security() has been called already.? If you are breaking that assumption, you have to fix it.

Yup. I misread the macro my first time around. Easy fix.

Is there a reason you can't make inode_alloc_security() return void since you moved the allocation to the framework? 

No reason with any of the existing modules, But I could see someone
doing unnatural things during allocation that might result in a
failure.

Unfortunate that inode_init_security name is already in use for another purpose since essentially you have reduced these hooks to initialization only.

I considered that but decided that it makes more sense for the module hook names
to match the infrastructure name. Having security_inode_alloc() call
selinux_inode_setup_security() starts to get confusing.