Re: sleep in selinux_audit_rule_init

[Stephen Smalley <sds@tycho.nsa.gov>]

On 5/22/19 8:20 AM, Stephen Smalley wrote:
> On 5/22/19 7:49 AM, Janne Karhunen wrote:
>> Hi,
>>
>> I managed to hit a following BUG, looks like ima can call
>> selinux_audit_rule_init that can sleep in rcu critical section in
>> ima_match_policy():
>>
>> __might_sleep
>> kmem_cache_alloc_trace
>> selinux_audit_rule_init <<< kzalloc (.. GFP_KERNEL)
>> security_audit_rule_init
>> ima_match_policy <<< list_for_each_entry_rcu
>> ima_get_action
>> process_measurement
>> ima_file_check
>> path_openat
>> do_filp_open
>> ..
>>
>> I guess this is the ima_match_rules() calling ima_lsm_update_rules()
>> when it concludes that the selinux policy may have been reloaded.
>>
>> The easy way for me to fix my own butt in this regard is to change the
>> selinux allocation not to wait, but Paul would you be OK with such
>> change? The alternative looks like a pretty big change in the ima?
> 
> This is perhaps a sign of a deeper bug in IMA; if they are in the middle 
> of matching against their policy rules, then they shouldn't be 
> updating/modifying those rules in the middle of match processing?  How 
> is that safe under RCU?
> 
> If you look at how the audit subsystem deals with the same problem, they 
> have a callback (audit_update_lsm_rules) that is called upon an AVC 
> reset (hence upon a policy reload) and can update all of their rules at 
> that time, not lazily during matching.  Since that time, a more general 
> notifier mechanism was added, register_lsm_notifier(), and is used by 
> infiniband to update its state upon policy changes.

Another potentially worrisome aspect of the current 
ima_lsm_update_rules() logic is that it does a BUG_ON() if the attempt 
to update the rule fails, which could occur if e.g. one had an IMA 
policy rule based on a given domain/type and that domain/type were 
removed from policy (e.g. via policy module removal).  Contrast with the 
handling in audit_dupe_lsm_field().  The existing ima_lsm_update_rules() 
logic could also yield a BUG_ON upon transient memory allocation failure.