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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 3/2/20 7:38 AM, Eric W. Biederman wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>> This fixes a deadlock in the tracer when tracing a multi-threaded
>> application that calls execve while more than one thread are running.
>>
>> I observed that when running strace on the gcc test suite, it always
>> blocks after a while, when expect calls execve, because other threads
>> have to be terminated.  They send ptrace events, but the strace is no
>> longer able to respond, since it is blocked in vm_access.
>>
>> The deadlock is always happening when strace needs to access the
>> tracees process mmap, while another thread in the tracee starts to
>> execve a child process, but that cannot continue until the
>> PTRACE_EVENT_EXIT is handled and the WIFEXITED event is received:
> 
> I think your patch works, but I don't think to solve your case another
> mutex is necessary.  Possibly it is justified, but I hesitate to
> introduce yet another concept in the code.
> 
> Having read elsewhere in the thread that this does not solve the problem
> Oleg has mentioned I am really hesitant to add more complexity to the
> situation.
> 
> 
> For your case there is a straight forward and local workaround.
> 
> When the current task is ptracing the target task don't bother with
> cred_gaurd_mutex and ptrace_may_access in access_mm as those tests
> have already passed.  Instead just confirm the ptrace status. AKA
> the permission check in ptraces_access_vm.
> 
> I think something like this is all we need.
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cee89229606a..b0ab98c84589 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1224,6 +1224,16 @@ struct mm_struct *mm_access(struct task_struct *task, unsigned int mode)
>  	struct mm_struct *mm;
>  	int err;
>  
> +	if (task->ptrace && (current == task->parent)) {
> +		mm = get_task_mm(task);
> +		if ((get_dumpable(mm) != SUID_DUMP_USER) &&
> +		    !ptracer_capable(task, mm->user_ns)) {
> +			mmput(mm);
> +			mm = ERR_PTR(-EACCESS);
> +		}
> +		return mm;
> +	}
> +
>  	err =  mutex_lock_killable(&task->signal->cred_guard_mutex);
>  	if (err)
>  		return ERR_PTR(err);
> 
> Does this solve your test case?
> 

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.  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.


Bernd.