linux-kernel - Re: [PATCH 6/6] ptrace: PTRACE_DETACH should do flush_ptrace_hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 29 Apr 2013 18:09:33 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Ingo Molnar <mingo@...e.hu>,
	Jan Kratochvil <jan.kratochvil@...hat.com>,
	Prasad <prasad@...ux.vnet.ibm.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] ptrace: PTRACE_DETACH should do
 flush_ptrace_hw_breakpoint(child)

On Thu, Apr 18, 2013 at 08:44:25PM +0200, Oleg Nesterov wrote:
> Change ptrace_detach() to call flush_ptrace_hw_breakpoint(child).
> This frees the slots for non-ptrace PERF_TYPE_BREAKPOINT users, and
> this ensures that the tracee won't be killed by SIGTRAP triggered by
> the active breakpoints.
> 
> Test-case:
> 
> 	unsigned long encode_dr7(int drnum, int enable, unsigned int type, unsigned int len)
> 	{
> 		unsigned long dr7;
> 
> 		dr7 = ((len | type) & 0xf)
> 			<< (DR_CONTROL_SHIFT + drnum * DR_CONTROL_SIZE);
> 		if (enable)
> 			dr7 |= (DR_GLOBAL_ENABLE << (drnum * DR_ENABLE_SIZE));
> 
> 		return dr7;
> 	}
> 
> 	int write_dr(int pid, int dr, unsigned long val)
> 	{
> 		return ptrace(PTRACE_POKEUSER, pid,
> 				offsetof (struct user, u_debugreg[dr]),
> 				val);
> 	}
> 
> 	void func(void)
> 	{
> 	}
> 
> 	int main(void)
> 	{
> 		int pid, stat;
> 		unsigned long dr7;
> 
> 		pid = fork();
> 		if (!pid) {
> 			assert(ptrace(PTRACE_TRACEME, 0,0,0) == 0);
> 			kill(getpid(), SIGHUP);
> 
> 			func();
> 			return 0x13;
> 		}
> 
> 		assert(pid == waitpid(-1, &stat, 0));
> 		assert(WSTOPSIG(stat) == SIGHUP);
> 
> 		assert(write_dr(pid, 0, (long)func) == 0);
> 		dr7 = encode_dr7(0, 1, DR_RW_EXECUTE, DR_LEN_1);
> 		assert(write_dr(pid, 7, dr7) == 0);
> 
> 		assert(ptrace(PTRACE_DETACH, pid, 0,0) == 0);
> 		assert(pid == waitpid(-1, &stat, 0));
> 		assert(stat == 0x1300);
> 
> 		return 0;
> 	}
> 
> Before this patch the child is killed after PTRACE_DETACH.
> 
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  kernel/ptrace.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 776ab3b..33752d9 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -467,6 +467,7 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
>  	/* Architecture-specific hardware disable .. */
>  	ptrace_disable(child);
>  	clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
> +	flush_ptrace_hw_breakpoint(child);

So I assume the tracee is still guaranteed to be stopped at that time, right? Or already
killed? But it can't be concurrently killed given the patch you did that prevented that?

I'm just asking to make sure we can't have two concurrent calls to flush_ptrace_hw_breakpoint()
at the same time.

Also it seems to be a regression since we brought the breakpoint/perf infrastructure. But
backporting this patch prior to "ptrace: ensure arch_ptrace/ptrace_request can never race with SIGKILL"
might be racy. Hmm...

Thanks.

>  
>  	write_lock_irq(&tasklist_lock);
>  	/*
> -- 
> 1.5.5.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/