From mboxrd@z Thu Jan 1 00:00:00 1970 From: Enke Chen Subject: Re: [PATCH] kernel/signal: Signal-based pre-coredump notification Date: Mon, 15 Oct 2018 14:31:57 -0700 Message-ID: <3e3728f7-1370-586c-c5ac-4ba0604976a5@cisco.com> References: <20181015222144.27fdafc3@alans-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20181015222144.27fdafc3@alans-desktop> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Alan Cox Cc: Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , x86@kernel.org, Peter Zijlstra , Arnd Bergmann , "Eric W. Biederman" , Khalid Aziz , Kate Stewart , Helge Deller , Greg Kroah-Hartman , Al Viro , Andrew Morton , Christian Brauner , Catalin Marinas , Will Deacon , Dave Martin , Mauro Carvalho Chehab , Michal Hocko , Rik List-Id: linux-arch.vger.kernel.org Hi, Alan: As I replied earlier, I will remove the logic that allows setting on others. This function "set_predump_signal_perm()" will be gone too. Thanks. -- Enke On 10/15/18 2:21 PM, Alan Cox wrote: >> +/* >> + * Returns true if current's euid is same as p's uid or euid, >> + * or has CAP_SYS_ADMIN. >> + * >> + * Called with rcu_read_lock, creds are safe. >> + * >> + * Adapted from set_one_prio_perm(). >> + */ >> +static bool set_predump_signal_perm(struct task_struct *p) >> +{ >> + const struct cred *cred = current_cred(), *pcred = __task_cred(p); >> + >> + return uid_eq(pcred->uid, cred->euid) || >> + uid_eq(pcred->euid, cred->euid) || >> + capable(CAP_SYS_ADMIN); >> +} > > This makes absolutely no security sense whatsoever. The uid and euid of > the parent and child can both change between the test and the signal > delivery. > > There are reasons that the child signal control code is incredibly > careful about either the parent or child using execve or doing a > privilege change that might pose a risk. > > Until this code gets the same protections I don't believe it's safe. > > Alan > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rcdn-iport-8.cisco.com ([173.37.86.79]:35624 "EHLO rcdn-iport-8.cisco.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725974AbeJPFYL (ORCPT ); Tue, 16 Oct 2018 01:24:11 -0400 Subject: Re: [PATCH] kernel/signal: Signal-based pre-coredump notification References: <20181015222144.27fdafc3@alans-desktop> From: Enke Chen Message-ID: <3e3728f7-1370-586c-c5ac-4ba0604976a5@cisco.com> Date: Mon, 15 Oct 2018 14:31:57 -0700 MIME-Version: 1.0 In-Reply-To: <20181015222144.27fdafc3@alans-desktop> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Alan Cox Cc: Thomas Gleixner , Ingo Molnar , Borislav Petkov , "H. Peter Anvin" , x86@kernel.org, Peter Zijlstra , Arnd Bergmann , "Eric W. Biederman" , Khalid Aziz , Kate Stewart , Helge Deller , Greg Kroah-Hartman , Al Viro , Andrew Morton , Christian Brauner , Catalin Marinas , Will Deacon , Dave Martin , Mauro Carvalho Chehab , Michal Hocko , Rik van Riel , "Kirill A. Shutemov" , Roman Gushchin , Marcos Paulo de Souza , Oleg Nesterov , Dominik Brodowski , Cyrill Gorcunov , Yang Shi , Jann Horn , Kees Cook , linux-kernel@vger.kernel.org, linux-arch@vger.kernel.org, "Victor Kamensky (kamensky)" , xe-linux-external@cisco.com, Stefan Strogin , Enke Chen Message-ID: <20181015213157.AqcFLHPWAgpPYsuA8ef3ktfdTnWzBCw2yNhpWhnGbZc@z> Hi, Alan: As I replied earlier, I will remove the logic that allows setting on others. This function "set_predump_signal_perm()" will be gone too. Thanks. -- Enke On 10/15/18 2:21 PM, Alan Cox wrote: >> +/* >> + * Returns true if current's euid is same as p's uid or euid, >> + * or has CAP_SYS_ADMIN. >> + * >> + * Called with rcu_read_lock, creds are safe. >> + * >> + * Adapted from set_one_prio_perm(). >> + */ >> +static bool set_predump_signal_perm(struct task_struct *p) >> +{ >> + const struct cred *cred = current_cred(), *pcred = __task_cred(p); >> + >> + return uid_eq(pcred->uid, cred->euid) || >> + uid_eq(pcred->euid, cred->euid) || >> + capable(CAP_SYS_ADMIN); >> +} > > This makes absolutely no security sense whatsoever. The uid and euid of > the parent and child can both change between the test and the signal > delivery. > > There are reasons that the child signal control code is incredibly > careful about either the parent or child using execve or doing a > privilege change that might pose a risk. > > Until this code gets the same protections I don't believe it's safe. > > Alan >