From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964906AbdEWFrg (ORCPT ); Tue, 23 May 2017 01:47:36 -0400 Received: from mx2.suse.de ([195.135.220.15]:57861 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S934600AbdEWFre (ORCPT ); Tue, 23 May 2017 01:47:34 -0400 Date: Tue, 23 May 2017 07:47:32 +0200 Message-ID: From: Takashi Iwai To: ebiederm@xmission.com (Eric W. Biederman) Cc: linux-kernel@vger.kernel.org Subject: Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork In-Reply-To: <877f18txfz.fsf_-_@xmission.com> References: <87r2zgtzbi.fsf@xmission.com> <877f18txfz.fsf_-_@xmission.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.2 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 22 May 2017 23:04:48 +0200, Eric W. Biederman wrote: > > > When I introduced ptracer_cred I failed to consider the weirdness of > fork where the task_struct copies the old value by default. This > winds up leaving ptracer_cred set even when a process forks and > the child process does not wind up being ptraced. > > Because ptracer_cred is not set on non-ptraced processes whose > parents were ptraced this has broken the ability of the enlightenment > window manager to start setuid children. > > Fix this by properly initializing ptracer_cred in ptrace_init_task > > This must be done with a little bit of care to preserve the current value > of ptracer_cred when ptrace carries through fork. Re-reading the > ptracer_cred from the ptracing process at this point is inconsistent > with how PT_PTRACE_CAP has been maintained all of these years. > > Fixes: 64b875f7ac8a ("ptrace: Capture the ptracer's creds not PT_PTRACE_CAP") > Signed-off-by: "Eric W. Biederman" > --- > > If I could get some folks to test and verify this fixes the > enlightenment issue I would really appreciate it. This seems giving a compile warning and it becomes error in the following: In file included from ./include/linux/mutex.h:13:0, from ./include/linux/kernfs.h:13, from ./include/linux/sysfs.h:15, from ./include/linux/kobject.h:21, from ./include/linux/device.h:17, from drivers/gpu/drm/i915/gvt/kvmgt.c:32: ./include/linux/ptrace.h: In function ‘ptrace_init_task’: ./arch/x86/include/asm/current.h:17:17: error: passing argument 3 of ‘__ptrace_link’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] #define current get_current() ^ ./include/linux/ptrace.h:210:41: note: in expansion of macro ‘current’ __ptrace_link(child, current->parent, current->ptracer_cred); ^~~~~~~ In file included from ./arch/x86/include/asm/stacktrace.h:10:0, from ./arch/x86/include/asm/perf_event.h:246, from ./include/linux/perf_event.h:24, from ./arch/x86/include/asm/kvm_host.h:24, from ./include/linux/kvm_host.h:37, from drivers/gpu/drm/i915/gvt/kvmgt.c:41: ./include/linux/ptrace.h:56:13: note: expected ‘struct cred *’ but argument is of type ‘const struct cred *’ extern void __ptrace_link(struct task_struct *child, ^~~~~~~~~~~~~ cc1: all warnings being treated as errors thanks, Takashi > > include/linux/ptrace.h | 7 +++++-- > kernel/ptrace.c | 20 +++++++++++++------- > 2 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h > index 422bc2e4cb6a..23c5716f7bd2 100644 > --- a/include/linux/ptrace.h > +++ b/include/linux/ptrace.h > @@ -54,7 +54,8 @@ extern int ptrace_request(struct task_struct *child, long request, > unsigned long addr, unsigned long data); > extern void ptrace_notify(int exit_code); > extern void __ptrace_link(struct task_struct *child, > - struct task_struct *new_parent); > + struct task_struct *new_parent, > + struct cred *ptracer_cred); > extern void __ptrace_unlink(struct task_struct *child); > extern void exit_ptrace(struct task_struct *tracer, struct list_head *dead); > #define PTRACE_MODE_READ 0x01 > @@ -206,7 +207,7 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace) > > if (unlikely(ptrace) && current->ptrace) { > child->ptrace = current->ptrace; > - __ptrace_link(child, current->parent); > + __ptrace_link(child, current->parent, current->ptracer_cred); > > if (child->ptrace & PT_SEIZED) > task_set_jobctl_pending(child, JOBCTL_TRAP_STOP); > @@ -215,6 +216,8 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace) > > set_tsk_thread_flag(child, TIF_SIGPENDING); > } > + else > + child->ptracer_cred = NULL; > } > > /** > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > index 266ddcc1d8bb..79cbe00fe787 100644 > --- a/kernel/ptrace.c > +++ b/kernel/ptrace.c > @@ -60,19 +60,25 @@ int ptrace_access_vm(struct task_struct *tsk, unsigned long addr, > } > > > +void __ptrace_link(struct task_struct *child, struct task_struct *new_parent, > + struct cred *ptracer_cred) > +{ > + BUG_ON(!list_empty(&child->ptrace_entry)); > + list_add(&child->ptrace_entry, &new_parent->ptraced); > + child->parent = new_parent; > + child->ptracer_cred = get_cred(ptracer_cred); > +} > + > /* > * ptrace a task: make the debugger its new parent and > * move it to the ptrace list. > * > * Must be called with the tasklist lock write-held. > */ > -void __ptrace_link(struct task_struct *child, struct task_struct *new_parent) > +static void ptrace_link(struct task_struct *child, struct task_struct *new_parent) > { > - BUG_ON(!list_empty(&child->ptrace_entry)); > - list_add(&child->ptrace_entry, &new_parent->ptraced); > - child->parent = new_parent; > rcu_read_lock(); > - child->ptracer_cred = get_cred(__task_cred(new_parent)); > + __ptrace_link(child, new_parent, __task_cred(new_parent)); > rcu_read_unlock(); > } > > @@ -386,7 +392,7 @@ static int ptrace_attach(struct task_struct *task, long request, > flags |= PT_SEIZED; > task->ptrace = flags; > > - __ptrace_link(task, current); > + ptrace_link(task, current); > > /* SEIZE doesn't trap tracee on attach */ > if (!seize) > @@ -459,7 +465,7 @@ static int ptrace_traceme(void) > */ > if (!ret && !(current->real_parent->flags & PF_EXITING)) { > current->ptrace = PT_PTRACED; > - __ptrace_link(current, current->real_parent); > + ptrace_link(current, current->real_parent); > } > } > write_unlock_irq(&tasklist_lock); > -- > 2.10.1 >