linux-kernel - Re: [PATCH v14] exec: Fix dead-lock in de_thread with ptrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <AS8P193MB12857F0E26BD5C8D1BC632F6E4722@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM>
Date: Wed, 17 Jan 2024 10:51:14 +0100
From: Bernd Edlinger <bernd.edlinger@...mail.de>
To: Matthew Wilcox <willy@...radead.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>,
 Alexey Dobriyan <adobriyan@...il.com>, Oleg Nesterov <oleg@...hat.com>,
 Kees Cook <keescook@...omium.org>, Andy Lutomirski <luto@...capital.net>,
 Will Drewry <wad@...omium.org>, Christian Brauner <brauner@...nel.org>,
 Andrew Morton <akpm@...ux-foundation.org>, Michal Hocko <mhocko@...e.com>,
 Serge Hallyn <serge@...lyn.com>, James Morris
 <jamorris@...ux.microsoft.com>, Randy Dunlap <rdunlap@...radead.org>,
 Suren Baghdasaryan <surenb@...gle.com>, Yafang Shao <laoar.shao@...il.com>,
 Helge Deller <deller@....de>, "Eric W. Biederman" <ebiederm@...ssion.com>,
 Adrian Reber <areber@...hat.com>, Thomas Gleixner <tglx@...utronix.de>,
 Jens Axboe <axboe@...nel.dk>, Alexei Starovoitov <ast@...nel.org>,
 "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
 tiozhang <tiozhang@...iglobal.com>, Luis Chamberlain <mcgrof@...nel.org>,
 "Paulo Alcantara (SUSE)" <pc@...guebit.com>,
 Sergey Senozhatsky <senozhatsky@...omium.org>,
 Frederic Weisbecker <frederic@...nel.org>, YueHaibing
 <yuehaibing@...wei.com>, Paul Moore <paul@...l-moore.com>,
 Aleksa Sarai <cyphar@...har.com>, Stefan Roesch <shr@...kernel.io>,
 Chao Yu <chao@...nel.org>, xu xin <xu.xin16@....com.cn>,
 Jeff Layton <jlayton@...nel.org>, Jan Kara <jack@...e.cz>,
 David Hildenbrand <david@...hat.com>, Dave Chinner <dchinner@...hat.com>,
 Shuah Khan <shuah@...nel.org>, Zheng Yejian <zhengyejian1@...wei.com>,
 Elena Reshetova <elena.reshetova@...el.com>,
 David Windsor <dwindsor@...il.com>, Mateusz Guzik <mjguzik@...il.com>,
 Ard Biesheuvel <ardb@...nel.org>,
 "Joel Fernandes (Google)" <joel@...lfernandes.org>,
 Hans Liljestrand <ishkamiel@...il.com>
Subject: Re: [PATCH v14] exec: Fix dead-lock in de_thread with ptrace_attach

On 1/15/24 20:37, Matthew Wilcox wrote:
> On Mon, Jan 15, 2024 at 08:22:19PM +0100, Bernd Edlinger wrote:
>> This introduces signal->exec_bprm, which is used to
>> fix the case when at least one of the sibling threads
>> is traced, and therefore the trace process may dead-lock
>> in ptrace_attach, but de_thread will need to wait for the
>> tracer to continue execution.
> 
> Not entirely sure why I've been added to the cc; this doesn't seem
> like it's even remotely within my realm of expertise.
> 

Ah, okay, never mind.
A couple new email addresses were found this time when I used
/scripts/get_maintainer.pl

>> +++ b/include/linux/cred.h
>> @@ -153,6 +153,7 @@ extern const struct cred *get_task_cred(struct task_struct *);
>>  extern struct cred *cred_alloc_blank(void);
>>  extern struct cred *prepare_creds(void);
>>  extern struct cred *prepare_exec_creds(void);
>> +extern bool is_dumpability_changed(const struct cred *, const struct cred *);
> 
> Using 'extern' for function declarations is deprecated.  More
> importantly, you have two arguments of the same type, and how do I know
> which one is which if you don't name them?
> 
>> +++ b/kernel/cred.c
>> @@ -375,6 +375,28 @@ static bool cred_cap_issubset(const struct cred *set, const struct cred *subset)
>>  	return false;
>>  }
>>  
>> +/**
>> + * is_dumpability_changed - Will changing creds from old to new
>> + * affect the dumpability in commit_creds?
>> + *
>> + * Return: false - dumpability will not be changed in commit_creds.
>> + *         true  - dumpability will be changed to non-dumpable.
>> + *
>> + * @old: The old credentials
>> + * @new: The new credentials
>> + */
> 
> Does kernel-doc really parse this correctly?  Normal style would be:

Apparently yes, but I think I only added those lines to silence
some automatic checking bots.

> 
> /**
>  * is_dumpability_changed - Will changing creds affect dumpability?
>  * @old: The old credentials.
>  * @new: The new credentials.
>  *
>  * If the @new credentials have no elevated privileges compared to the
>  * @old credentials, the task may remain dumpable.  Otherwise we have
>  * to mark the task as undumpable to avoid information leaks from higher
>  * to lower privilege domains.
>  *
>  * Return: True if the task will become undumpable.
>  */
> 

Thanks a lot, that looks much better. I will use your suggestion as is,
when I re-send the patch next time.

>> @@ -508,6 +531,14 @@ static int ptrace_traceme(void)
>>  {
>>  	int ret = -EPERM;
>>  
>> +	if (mutex_lock_interruptible(&current->signal->cred_guard_mutex))
>> +		return -ERESTARTNOINTR;
> 
> Do you really want this to be interruptible by a timer signal or a
> window resize event?
> 

I think that is kind of okay, as most of the existing users lock the mutex
also interruptible, so I just wanted to follow those examples.

Thanks
Bernd.