From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alan Cox Subject: Re: [PATCH] kernel/signal: Signal-based pre-coredump notification Date: Mon, 15 Oct 2018 22:21:44 +0100 Message-ID: <20181015222144.27fdafc3@alans-desktop> References: Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Enke Chen 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 > +/* > + * 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 www.llwyncelyn.cymru ([82.70.14.225]:35432 "EHLO fuzix.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725974AbeJPFKM (ORCPT ); Tue, 16 Oct 2018 01:10:12 -0400 Date: Mon, 15 Oct 2018 22:21:44 +0100 From: Alan Cox Subject: Re: [PATCH] kernel/signal: Signal-based pre-coredump notification Message-ID: <20181015222144.27fdafc3@alans-desktop> In-Reply-To: References: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Enke Chen 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 Message-ID: <20181015212144.OwvYW1UUKHSZEZqSg6zdTjcWfIcKf6f3sKu1Yswcnb8@z> > +/* > + * 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