On Thu, Oct 19, 2023 at 12:56 PM Mateusz Guzik <[email protected]> wrote:
> On 10/19/23, Paul Moore <[email protected]> wrote:
> > On Thu, Oct 19, 2023 at 10:52 AM Mateusz Guzik <[email protected]> wrote:
> >> On 10/19/23, Paul Moore <[email protected]> wrote:
> >> > Thinking about it a bit more this morning, I think we can safely
> >> > ignore the non-@current case in audit_exe_compare() as the whole point
> >> > of the audit exe filter is to record the actions of processes
> >> > instantiated from that executable file; if the @current task is not
> >> > being logged/filtered, we shouldn't have to worry about the exe
> >> > filter.
> >>
> >> I did a quick stab at figuring out whether one can get there with
> >> non-current to begin with, but did not convince myself it is not
> >> possible.
> >>
> >> That said, should you repost, I think refing and unrefing mm should be a
> >> voided.
> >
> > We have to deref current->mm to get the exe_file, but so long as we
> > get a reference with mmget()/mmput() it should be safe, no?
>
> For task == current the very condition which allows you to safely
> mmget also makes the operation redundant -- current already has a ref
> on mm for as long as it executes.
I've been using the move_pages(2) syscall code as an example and in
the find_mm_struct() function the code either does a mmget() if
accessing current->mm or a get_task_mm() if accessing an arbitrary
task.
-> SYSCALL_DEFINE6(move_pages, ...)
-> kernel_move_pages(...)
-> find_mm_struct(...)
What am I missing?
> >> The bug showed up with 18c91bb2d87268d23868bf13508f5bc9cf04e89a
> >> ("prlimit: do not grab the tasklist_lock") which converted that lock
> >> to task_lock. So I don't think pointing at my patch as "Fixes" is
> >> accurate, but I'm not going to insist. ;)
> >
> > Hmm, 18c91bb2d872 doesn't look like it adds a call to task_lock(), did
> > you copy-n-past the wrong commit or am I missing something? From what
> > I can see, the task_lock() was first introduced back in 2009 with
> > 86f162f4c75c ("rlimits: do security check under task_lock").
> >
>
> Huh. That's some brainfarting by me, indeed with this in place it is
> my commit which regresses.
No worries, thanks for the confirmation.
--
paul-moore.com
