On 10/19/23, Paul Moore <[email protected]> wrote: > On Wed, Oct 18, 2023 at 8:22 PM Mateusz Guzik <[email protected]> wrote: >> On 10/19/23, Paul Moore <[email protected]> wrote: >> >> The get_task_exe_file() function locks the given task with task_lock() >> >> which when used inside audit_exe_compare() can cause deadlocks on >> >> systems that generate audit records when the task_lock() is held. As >> >> we should be able to safely access current->mm we can drop the call to >> >> get_task_exe_file() and get a reference to current->mm directly which >> >> we can then use in a call to get_mm_exe_file() without having to take >> >> task_lock(). >> >> I don't follow what's the deadlock. >> >> You mean they hold task lock for current and then call into >> audit_exe_compare which takes it again? >> >> I would argue any calls into the routine with *any* task already >> locked are fishy at best (I'm assuming there is an established task >> lock ordering which may be accidentally violated). > > A link to the original problem report is below, but the quick summary > is a capable() check in the do_prlimit() code path. > > https://lore.kernel.org/audit/[email protected] >
While get_task_exe_file working without taking the task lock would be nice, I think the *real* bug is do_prlimit calling into capabilities/security machinery with said lock held to begin with. Taking 2 task locks taken in an arbitrary order sounds incredibly dodgy and is probably already illegal. Even with the proposed patch it would happen with do_prlimit invoked by non-group leader. With that assumption, if one tries to argue that calling into capabilities() and security_* hooks with a task lock is legal, then none of the code inside can take the lock, which sounds like an unsustainable limitation (even if it so happens that audit calling into get_task_exe_file is the only case at the moment). All that said I think the cleanest way forward is to add a dedicated spinlock for rlimits and stuff it into signal_struct. I'm going to hack it up tomorrow, but some more people will need to be added to the discussion. -- Mateusz Guzik <mjguzik gmail.com>
