Re: [PATCHv2] exec: Fix a deadlock in ptrace

[christian]

<linux-doc@xxxxxxxxxxxxxxx>,"linux-kernel@xxxxxxxxxxxxxxx" <linux-kernel@xxxxxxxxxxxxxxx>,"linux-fsdevel@xxxxxxxxxxxxxxx" <linux-fsdevel@xxxxxxxxxxxxxxx>,"linux-mm@xxxxxxxxx" <linux-mm@xxxxxxxxx>,"stable@xxxxxxxxxxxxxxx" <stable@xxxxxxxxxxxxxxx>,linux-security-module <linux-security-module@xxxxxxxxxxxxxxx>
From: Christian Brauner <christian.brauner@xxxxxxxxxx>
Message-ID: <9C3BF644-0F82-48C9-9116-8554204FB57D@xxxxxxxxxx>

On March 2, 2020 6:37:27 PM GMT+01:00, Jann Horn <jannh@xxxxxxxxxx> wrote:
>On Mon, Mar 2, 2020 at 6:01 PM Bernd Edlinger
><bernd.edlinger@xxxxxxxxxx> wrote:
>> On 3/2/20 5:43 PM, Jann Horn wrote:
>> > On Mon, Mar 2, 2020 at 5:19 PM Eric W. Biederman
><ebiederm@xxxxxxxxxxxx> wrote:
>> >>
>> >> Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes:
>> >>
>> >>> On 3/2/20 4:57 PM, Eric W. Biederman wrote:
>> >>>> Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes:
>> >>>>
>> >>>>>
>> >>>>> I tried this with s/EACCESS/EACCES/.
>> >>>>>
>> >>>>> The test case in this patch is not fixed, but strace does not
>freeze,
>> >>>>> at least with my setup where it did freeze repeatable.
>> >>>>
>> >>>> Thanks, That is what I was aiming at.
>> >>>>
>> >>>> So we have one method we can pursue to fix this in practice.
>> >>>>
>> >>>>> That is
>> >>>>> obviously because it bypasses the cred_guard_mutex.  But all
>other
>> >>>>> process that access this file still freeze, and cannot be
>> >>>>> interrupted except with kill -9.
>> >>>>>
>> >>>>> However that smells like a denial of service, that this
>> >>>>> simple test case which can be executed by guest, creates a
>/proc/$pid/mem
>> >>>>> that freezes any process, even root, when it looks at it.
>> >>>>> I mean: "ln -s README /proc/$pid/mem" would be a nice bomb.
>> >>>>
>> >>>> Yes.  Your the test case in your patch a variant of the original
>> >>>> problem.
>> >>>>
>> >>>>
>> >>>> I have been staring at this trying to understand the
>fundamentals of the
>> >>>> original deeper problem.
>> >>>>
>> >>>> The current scope of cred_guard_mutex in exec is because being
>ptraced
>> >>>> causes suid exec to act differently.  So we need to know early
>if we are
>> >>>> ptraced.
>> >>>>
>> >>>
>> >>> It has a second use, that it prevents two threads entering
>execve,
>> >>> which would probably result in disaster.
>> >>
>> >> Exec can fail with an error code up until de_thread.  de_thread
>causes
>> >> exec to fail with the error code -EAGAIN for the second thread to
>get
>> >> into de_thread.
>> >>
>> >> So no.  The cred_guard_mutex is not needed for that case at all.
>> >>
>> >>>> If that case did not exist we could reduce the scope of the
>> >>>> cred_guard_mutex in exec to where your patch puts the
>cred_change_mutex.
>> >>>>
>> >>>> I am starting to think reworking how we deal with ptrace and
>exec is the
>> >>>> way to solve this problem.
>> >>
>> >>
>> >> I am 99% convinced that the fix is to move cred_guard_mutex down.
>> >
>> > "move cred_guard_mutex down" as in "take it once we've already set
>up
>> > the new process, past the point of no return"?
>> >
>> >> Then right after we take cred_guard_mutex do:
>> >>         if (ptraced) {
>> >>                 use_original_creds();
>> >>         }
>> >>
>> >> And call it a day.
>> >>
>> >> The details suck but I am 99% certain that would solve everyones
>> >> problems, and not be too bad to audit either.
>> >
>> > Ah, hmm, that sounds like it'll work fine at least when no LSMs are
>involved.
>> >
>> > SELinux normally doesn't do the execution-degrading thing, it just
>> > blocks the execution completely - see their
>selinux_bprm_set_creds()
>> > hook. So I think they'd still need to set some state on the task
>that
>> > says "we're currently in the middle of an execution where the
>target
>> > task will run in context X", and then check against that in the
>> > ptrace_may_access hook. Or I suppose they could just kill the task
>> > near the end of execve, although that'd be kinda ugly.
>> >
>>
>> We have current->in_execve for that, right?
>> I think when the cred_guard_mutex is taken only in the critical
>section,
>> then PTRACE_ATTACH could take the guard_mutex, and look at
>current->in_execve,
>> and just return -EAGAIN in that case, right, everybody happy :)
>
>It's probably going to mean that things like strace will just randomly
>fail to attach to processes if they happen to be in the middle of
>execve... but I guess that works?

That sounds like an acceptable outcome.
We can at least risk it and if we regress
revert or come up with the more complex
solution suggested in another mail here?