On 10/19/23, Mateusz Guzik <[email protected]> wrote: > 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. >
sigh, now that I wrote I remembered prlimit operates on arbitrary threads to begin with. I'm going to have to sleep on it. -- Mateusz Guzik <mjguzik gmail.com>
