On Thu, 2004-12-09 at 09:46 -0500, Stephen Smalley wrote: > Hi, > > The discussion of bprm_apply_creds reminded me of another issue with > respect to stacking: the holding of task_lock(current) across the > bprm_apply_creds() call. Currently, SELinux invokes the secondary_ops > hook first and then performs its own checking and task security > structure update, and then releases the task lock prior to performing > the rest of its processing, as we do not need the task lock for the rest > of the processing and holding for that processing would introduce > additional nesting of locks that is undesirable. Then, prior to > returning, SELinux re-takes the task lock so that everything works > smoothly when compute_creds tries to release it. > > This becomes a problem if you want to stack another module with SELinux > via stacker that also performs processing in bprm_apply_creds() and > relies on the unsafe flags, as they may no longer be accurate due to the > temporary dropping of the task lock by SELinux. You essentially have to > make SELinux the last module in the stacker chain to make that safe. > But that is difficult given that SELinux must be built-in, unless there > is an explicit way for SELinux to ask to be placed last (e.g. a priority > for registration) or stacker knows that SELinux should always go last. David Wheeler's original stacker allowed modules to be inserted various ways. There's certainly no reason why we couldn't allow modules to be inserted at the front of the list, or before the last module. But I think I like the second option better. > The other option would be to split the bprm_apply_creds hook into two > separate hooks, and only hold the task lock across the first hook call, > moving the SELinux processing that doesn't require the task lock into > the second hook function. The first hook would need to pass back some > state to the caller to propagate to the second hook. In the case of > SELinux, we would need to pass the information needed to perform the > avc_audit() calls for the share and ptrace checks. That would certainly > be cleaner, but again introduces a situation where we are making two > separate hook calls at the same call site. Would the attached patch be sufficient, or should the force_sig_specific and avc_audit also be moved to the final_setup()? It doesn't look to me like doing them under task_lock is a problem, and it did test fine, but I don't think I actually triggered the test conditions. (How do I get selinux to try an unlawful transition?) Proposed patch attached, as is the resulting relevant portion of selinux/hooks.c, since the patch is kind of hard to read there. thanks, -serge -- Serge Hallyn <serue@private>
This archive was generated by hypermail 2.1.3 : Thu Dec 09 2004 - 16:34:17 PST