[4.11 regression] su / sudo doesn't work when enlightenment is running as the window manager

[4.11 regression] su / sudo doesn't work when enlightenment is running as the window manager

[Takashi Iwai]

Hi Eric,

we've got a bug report showing the weird behavior supposedly triggered
by your commit 20523132ec5d ("exec: Test the ptracer's saved cred to
see if the tracee can gain caps").  In short, user can't run su or
sudo any longer when a terminal is started from Enlighentment window
manager.

Some details are found in openSUSE Bugzilla:
  https://bugzilla.suse.com/show_bug.cgi?id=1040041

and more in gentoo forum and Enlightenment bug tracker:
  https://forums.gentoo.org/viewtopic-t-1063022-postdays-0-postorder-asc-start-0.html
  https://phab.enlightenment.org/T5470

As the bug seems solely in Enlightenment, it might that be some its
specific patch is broken.  But a regression is a regression, after
all...

Could you take a look at the issue?

Thanks!


Takashi

Re: [4.11 regression] su / sudo doesn't work when enlightenment is running as the window manager

[Takashi Iwai]

On Mon, 22 May 2017 22:24:17 +0200,
Eric W. Biederman wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > Hi Eric,
> >
> > we've got a bug report showing the weird behavior supposedly triggered
> > by your commit 20523132ec5d ("exec: Test the ptracer's saved cred to
> > see if the tracee can gain caps").  In short, user can't run su or
> > sudo any longer when a terminal is started from Enlighentment window
> > manager.
> >
> > Some details are found in openSUSE Bugzilla:
> >   https://bugzilla.suse.com/show_bug.cgi?id=1040041
> >
> > and more in gentoo forum and Enlightenment bug tracker:
> >   https://forums.gentoo.org/viewtopic-t-1063022-postdays-0-postorder-asc-start-0.html
> >   https://phab.enlightenment.org/T5470
> >
> > As the bug seems solely in Enlightenment, it might that be some its
> > specific patch is broken.  But a regression is a regression, after
> > all...
> >
> > Could you take a look at the issue?
> 
> I will.
> 
> I am wondering if the bisect did not quite go back far enough as that
> change really should have been had no effect and it was the introduction
> of the ptracer_capable test that is causing problems.
> 
> To be able to think this through clearly I really need to understand
> what enlightenment-start is doing.
> 
> At first glance the reported behavior does seem corect.  If you are not
> root and you are ptracing a setuid-root application it should not have
> be run setuid.
> 
> So either I made a stilly mistake somewhere or enlightenment is doing
> something problematic.  The ugly possibility is that I might have closed
> a security hole they were depending upon by accident.
> 
> I took a quick look at what I think is the code to enlightment_start.
> AKA src/bin/e_start_main.c
> 
> And all it does is:
> 	pid = fork();
>         if (pid == 0) {
>         	/* child */
>                 ptrace(PT_TRACE_ME, 0, NULL);
>         } else {
>         	ptrace(PT_ATTACH, pid, NULL, NULL);
>                 ...;
>         }
> 
> So the tracer_cred should be the same on either path.
> 
> So if someone who understands what enlightenment is doing could boil
> this down or otherwise help me understand exactly what kind of ptrace
> interaction is causing this problem that would really help.
> 
> Hmm. You know I think I know what is going on and it is a bit
> embarrasing:
> 
> Can someone please test this change?  I am guessing I just forgot to zero
> ptracer_cred on fork...  Why ptrace_init_task lives in a header
> instead of in kernel/fork.c I do not know.

Thanks for a prompt reaction!

I'll prepare a test kernel and ask reporters giving it a try.


Takashi

> 
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 422bc2e4cb6a..a31442c4b272 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -202,6 +202,7 @@ static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
>         INIT_LIST_HEAD(&child->ptraced);
>         child->jobctl = 0;
>         child->ptrace = 0;
> +       child->ptracer_cred = NULL;
>         child->parent = child->real_parent;
>  
>         if (unlikely(ptrace) && current->ptrace) {
> 
> Eric
> 

[CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[Eric W. Biederman]

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" <ebiederm@xmission.com>
---

If I could get some folks to test and verify this fixes the
enlightenment issue I would really appreciate it.

 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

Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[Takashi Iwai]

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" <ebiederm@xmission.com>
> ---
> 
> 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
> 

Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8491 bytes --]

Hi Eric,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/ptrace-Properly-initialize-ptracer_cred-on-fork/20170523-143551
config: ia64-allnoconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   In file included from arch/ia64/include/asm/ptrace.h:44:0,
                    from arch/ia64/include/asm/processor.h:19,
                    from arch/ia64/include/asm/thread_info.h:11,
                    from include/linux/thread_info.h:37,
                    from include/asm-generic/preempt.h:4,
                    from ./arch/ia64/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:80,
                    from include/linux/spinlock.h:50,
                    from include/linux/seqlock.h:35,
                    from include/linux/time.h:5,
                    from include/linux/stat.h:18,
                    from include/linux/module.h:10,
                    from init/main.c:16:
   include/linux/ptrace.h: In function 'ptrace_init_task':
>> arch/ia64/include/asm/current.h:15:17: warning: passing argument 3 of '__ptrace_link' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    #define current ((struct task_struct *) ia64_getreg(_IA64_REG_TP))
                    ^
   include/linux/ptrace.h:210:41: note: in expansion of macro 'current'
      __ptrace_link(child, current->parent, current->ptracer_cred);
                                            ^~~~~~~
   In file included from include/linux/ftrace.h:13:0,
                    from include/linux/perf_event.h:47,
                    from include/linux/trace_events.h:9,
                    from include/trace/syscall.h:6,
                    from include/linux/syscalls.h:82,
                    from init/main.c:20:
   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,
                ^~~~~~~~~~~~~
--
   In file included from arch/ia64/include/asm/ptrace.h:44:0,
                    from arch/ia64/include/asm/processor.h:19,
                    from arch/ia64/include/asm/thread_info.h:11,
                    from include/linux/thread_info.h:37,
                    from include/asm-generic/preempt.h:4,
                    from ./arch/ia64/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:80,
                    from include/linux/spinlock.h:50,
                    from include/linux/wait.h:8,
                    from include/linux/fs.h:5,
                    from arch/ia64/kernel/sys_ia64.c:9:
   include/linux/ptrace.h: In function 'ptrace_init_task':
>> arch/ia64/include/asm/current.h:15:17: warning: passing argument 3 of '__ptrace_link' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    #define current ((struct task_struct *) ia64_getreg(_IA64_REG_TP))
                    ^
   include/linux/ptrace.h:210:41: note: in expansion of macro 'current'
      __ptrace_link(child, current->parent, current->ptracer_cred);
                                            ^~~~~~~
   In file included from include/linux/ftrace.h:13:0,
                    from include/linux/perf_event.h:47,
                    from include/linux/trace_events.h:9,
                    from include/trace/syscall.h:6,
                    from include/linux/syscalls.h:82,
                    from arch/ia64/kernel/sys_ia64.c:18:
   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,
                ^~~~~~~~~~~~~
   arch/ia64/kernel/sys_ia64.c: In function 'arch_get_unmapped_area':
   arch/ia64/kernel/sys_ia64.c:31:20: warning: unused variable 'mm' [-Wunused-variable]
     struct mm_struct *mm = current->mm;
                       ^~
--
   In file included from include/linux/sched.h:11:0,
                    from kernel/ptrace.c:12:
   include/linux/ptrace.h: In function 'ptrace_init_task':
>> arch/ia64/include/asm/current.h:15:17: warning: passing argument 3 of '__ptrace_link' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    #define current ((struct task_struct *) ia64_getreg(_IA64_REG_TP))
                    ^
   include/linux/ptrace.h:210:41: note: in expansion of macro 'current'
      __ptrace_link(child, current->parent, current->ptracer_cred);
                                            ^~~~~~~
   In file included from kernel/ptrace.c:20:0:
   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,
                ^~~~~~~~~~~~~
   In file included from include/linux/rculist.h:10:0,
                    from include/linux/pid.h:4,
                    from include/linux/sched.h:13,
                    from kernel/ptrace.c:12:
   kernel/ptrace.c: In function 'ptrace_link':
   include/linux/rcupdate.h:578:1: warning: passing argument 3 of '__ptrace_link' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    ({ \
    ^
   include/linux/rcupdate.h:701:2: note: in expansion of macro '__rcu_dereference_check'
     __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
     ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:760:28: note: in expansion of macro 'rcu_dereference_check'
    #define rcu_dereference(p) rcu_dereference_check(p, 0)
                               ^~~~~~~~~~~~~~~~~~~~~
   include/linux/cred.h:297:2: note: in expansion of macro 'rcu_dereference'
     rcu_dereference((task)->real_cred)
     ^~~~~~~~~~~~~~~
   kernel/ptrace.c:81:35: note: in expansion of macro '__task_cred'
     __ptrace_link(child, new_parent, __task_cred(new_parent));
                                      ^~~~~~~~~~~
   kernel/ptrace.c:63:6: note: expected 'struct cred *' but argument is of type 'const struct cred *'
    void __ptrace_link(struct task_struct *child, struct task_struct *new_parent,
         ^~~~~~~~~~~~~

vim +15 arch/ia64/include/asm/current.h

^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   1  #ifndef _ASM_IA64_CURRENT_H
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   2  #define _ASM_IA64_CURRENT_H
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   3  
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   4  /*
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   5   * Modified 1998-2000
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   6   *	David Mosberger-Tang <davidm@hpl.hp.com>, Hewlett-Packard Co
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   7   */
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   8  
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16   9  #include <asm/intrinsics.h>
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  10  
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  11  /*
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  12   * In kernel mode, thread pointer (r13) is used to point to the current task
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  13   * structure.
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  14   */
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16 @15  #define current	((struct task_struct *) ia64_getreg(_IA64_REG_TP))
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  16  
^1da177e include/asm-ia64/current.h Linus Torvalds 2005-04-16  17  #endif /* _ASM_IA64_CURRENT_H */

:::::: The code at line 15 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5989 bytes --]

Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3038 bytes --]

Hi Eric,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc2 next-20170523]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eric-W-Biederman/ptrace-Properly-initialize-ptracer_cred-on-fork/20170523-143551
config: alpha-defconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   In file included from ./arch/alpha/include/generated/asm/current.h:1:0,
                    from include/linux/wait.h:10,
                    from include/linux/mmzone.h:9,
                    from include/linux/gfp.h:5,
                    from include/linux/mm.h:9,
                    from include/linux/pagemap.h:7,
                    from arch/alpha/mm/init.c:9:
   include/linux/ptrace.h: In function 'ptrace_init_task':
>> include/asm-generic/current.h:6:23: error: passing argument 3 of '__ptrace_link' discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
    #define get_current() (current_thread_info()->task)
                          ^
   include/asm-generic/current.h:7:17: note: in expansion of macro 'get_current'
    #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/alpha/mm/init.c:16:0:
   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

vim +6 include/asm-generic/current.h

aafe4dbe Arnd Bergmann 2009-05-13  1  #ifndef __ASM_GENERIC_CURRENT_H
aafe4dbe Arnd Bergmann 2009-05-13  2  #define __ASM_GENERIC_CURRENT_H
aafe4dbe Arnd Bergmann 2009-05-13  3  
aafe4dbe Arnd Bergmann 2009-05-13  4  #include <linux/thread_info.h>
aafe4dbe Arnd Bergmann 2009-05-13  5  
aafe4dbe Arnd Bergmann 2009-05-13 @6  #define get_current() (current_thread_info()->task)
aafe4dbe Arnd Bergmann 2009-05-13  7  #define current get_current()
aafe4dbe Arnd Bergmann 2009-05-13  8  
aafe4dbe Arnd Bergmann 2009-05-13  9  #endif /* __ASM_GENERIC_CURRENT_H */

:::::: The code at line 6 was first introduced by commit
:::::: aafe4dbed0bf6cbdb2e9f03e1d42f8a540d8541d asm-generic: add generic versions of common headers

:::::: TO: Arnd Bergmann <arnd@arndb.de>
:::::: CC: Arnd Bergmann <arnd@klappe2.(none)>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12491 bytes --]

Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[Takashi Iwai]

On Tue, 23 May 2017 07:47:32 +0200,
Takashi Iwai wrote:
> 
> 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" <ebiederm@xmission.com>
> > ---
> > 
> > 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

Through a quick test on VM (fixed patch by adding const to
__ptrace_link() argument), it seems working fine.

Tested-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[Eric W. Biederman]

Takashi Iwai <tiwai@suse.de> writes:

> On Tue, 23 May 2017 07:47:32 +0200,
> Takashi Iwai wrote:
>> 
>> 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" <ebiederm@xmission.com>
>> > ---
>> > 
>> > 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
>
> Through a quick test on VM (fixed patch by adding const to
> __ptrace_link() argument), it seems working fine.

Thank you.  It seems when I fixed the const issue in my tree I forget
commit --amend so the fix didn't make it into the patch I sent out.

> Tested-by: Takashi Iwai <tiwai@suse.de>

Just to confirm.  You were able to reproduce the enlightenment failure
and this fixes it?

Eric

Re: [CFT][PATCH] ptrace: Properly initialize ptracer_cred on fork

[Takashi Iwai]

On Tue, 23 May 2017 14:40:58 +0200,
Eric W. Biederman wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > On Tue, 23 May 2017 07:47:32 +0200,
> > Takashi Iwai wrote:
> >> 
> >> 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" <ebiederm@xmission.com>
> >> > ---
> >> > 
> >> > 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
> >
> > Through a quick test on VM (fixed patch by adding const to
> > __ptrace_link() argument), it seems working fine.
> 
> Thank you.  It seems when I fixed the const issue in my tree I forget
> commit --amend so the fix didn't make it into the patch I sent out.
> 
> > Tested-by: Takashi Iwai <tiwai@suse.de>
> 
> Just to confirm.  You were able to reproduce the enlightenment failure
> and this fixes it?

Yes.  It was reproducible on KVM image.


Takashi