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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 Nov 2009 18:32:07 +0530 From: "K.Prasad" <prasad@...ux.vnet.ibm.com> To: Frederic Weisbecker <fweisbec@...il.com> Cc: Ingo Molnar <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>, Li Zefan <lizf@...fujitsu.com>, Alan Stern <stern@...land.harvard.edu>, Peter Zijlstra <peterz@...radead.org>, Arnaldo Carvalho de Melo <acme@...hat.com>, Steven Rostedt <rostedt@...dmis.org>, Jan Kiszka <jan.kiszka@....de>, Jiri Slaby <jirislaby@...il.com>, Avi Kivity <avi@...hat.com>, Paul Mackerras <paulus@...ba.org>, Mike Galbraith <efault@....de>, Masami Hiramatsu <mhiramat@...hat.com>, Paul Mundt <lethal@...ux-sh.org>, Arjan van de Ven <arjan@...ux.intel.com> Subject: Re: [PATCH 5/7 v6] hw-breakpoints: Rewrite the hw-breakpoints layer on top of perf events On Sun, Nov 08, 2009 at 04:28:59PM +0100, Frederic Weisbecker wrote: There were a few comments that I posted against version 6 of your patchset (which happened to cross your version 7 posting...) regarding the breakpoint interfaces, reservation of register for unpinned events and such... By the way, I'm looking at refs/heads/perfevents/hw-breakpoint branch in git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git and hope that's correct/latest? Some more comments about the ptrace implementation here... static int ptrace_set_breakpoint_addr(struct task_struct *tsk, int nr, unsigned long addr) { struct perf_event *bp; struct thread_struct *t = &tsk->thread; if (!t->ptrace_bps[nr]) { /* * Put stub len and type to register (reserve) an inactive but * correct bp */ bp = register_user_hw_breakpoint(addr, HW_BREAKPOINT_LEN_1, HW_BREAKPOINT_W, ptrace_triggered, tsk, false); .. ... } Given that a register_user_hw_breakpoint() is done at the time of a write to DR0-DR3, it would needlessly hold onto the debug register until the corresponding DR7 bit is allocated while using up one 'pinned' debug slot. It would be prudent to postpone the breakpoint registration till DR7 is changed to activate it. static int ptrace_write_dr7(struct task_struct *tsk, unsigned long data) { .. ... /* * We shoud have at least an inactive breakpoint at this * slot. It means the user is writing dr7 without having * written the address register first */ if (!bp) { rc = -EINVAL; break; } I was just about confused...thinking that the above condition would become true during second_pass, but alas it turns out that you restore "thread->ptrace_bps[i] = bp" again later. rc = arch_bp_generic_fields(len, type, &gen_len, &gen_type); if (rc) break; /* * This is a temporary thing as bp is unregistered/registered * to simulate modification */ bp = modify_user_hw_breakpoint(bp, bp->attr.bp_addr, gen_len, gen_type, bp->callback, tsk, true); modify_user_hw_breakpoint() is called twice (once per pass) and in its current implementation, it would leave open a window for register grabbing on two occasions. Another reason to change its implementation soon... thread->ptrace_bps[i] = NULL; Why not remove this line from here... if (!bp) { /* incorrect bp, or we have a bug in bp API */ rc = -EINVAL; break; } if (IS_ERR(bp)) { rc = PTR_ERR(bp); bp = NULL; break; } thread->ptrace_bps[i] = bp; ...and put it here inside a condition "if (second_pass)"? } /* * Make a second pass to free the remaining unused breakpoints * or to restore the original breakpoints if an error occurred. */ if (!second_pass) { second_pass = 1; if (rc < 0) { orig_ret = rc; data = old_dr7; } goto restore; } return ((orig_ret < 0) ? orig_ret : rc); } Thanks, K.Prasad -- 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/
Powered by blists - more mailing lists