Re: [PATCH v2 1/8] exec: introduce cred_guard_light

[Jann Horn <jann@thejh.net>]

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

On Fri, Sep 30, 2016 at 05:35:05PM +0200, Oleg Nesterov wrote:
> On 09/23, Jann Horn wrote:
> >
> > This is a new per-threadgroup lock that can often be taken instead of
> > cred_guard_mutex and has less deadlock potential.
> 
> Oh, please don't.
> 
> > I'm doing this because
> > Oleg Nesterov mentioned the potential for deadlocks, in particular if a
> > debugged task is stuck in execve, trying to get rid of a ptrace-stopped
> > thread, and the debugger attempts to inspect procfs files of the debugged
> > task.
> 
> Yes, but we need to fix this anyway. And I am not sure the new mutex can
> actually help.
> 
> And I think that cred_guard_mutex is already over-used in fs/proc. Say,
> I think lock_trace() must die, I simply can't understand why it is useful.

IMO it's just about reducing potential (really small) information leaks.
These leaks probably don't matter much in practice - they're racy and only
disclose minimal amounts of data -, but in principle, they expose data.

But since you're opposed to it and I don't see a significant benefit in
having these checks, I'll just remove the lock_trace() stuff from my patch
for now.

> Suppose we modify, say, proc_pid_stack() to do
> 
> 	save_stack_trace_tsk(task, &trace);
> 	if (!ptrace_may_access(task, ...))
> 		goto return -EPERM;
> 
> 	for (i = 0; i < trace.nr_entries; i++)
> 		seq_printf(...);
> 
> 	return 0;
> 
> is there any problem if it shows some trace before setuid exec does
> install_exec_creds() ?
> 
> Oleg.
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]