Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

powerpc/watchpoints: Track perf single step directly on the breakpoint

There is a bug in the current watchpoint tracking logic, where the
teardown in arch_unregister_hw_breakpoint() uses bp->ctx->task, which it
does not have a reference of and parallel threads may be in the process
of destroying. This was partially addressed in commit fb822e6076d9
("powerpc/hw_breakpoint: Fix oops when destroying hw_breakpoint event"),
but the underlying issue of accessing a struct member in an unknown
state still remained. Syzkaller managed to trigger a null pointer
derefernce due to the race between the task destructor and checking the
pointer and dereferencing it in the loop.

While this null pointer dereference could be fixed by using READ_ONCE
to access the task up front, that just changes the error to manipulating
possbily freed memory.

Instead, the breakpoint logic needs to be reworked to remove any
dependency on a context or task struct during breakpoint removal.

The reason we have this currently is to clear thread.last_hit_ubp. This
member is used to differentiate the perf DAWR single-step sequence from
other causes of single-step, such as userspace just calling
ptrace(PTRACE_SINGLESTEP, ...). We need to differentiate them because,
when the single step interrupt is received, we need to know whether to
re-insert the DAWR breakpoint (perf) or not (ptrace / other).

arch_unregister_hw_breakpoint() needs to clear this information to
prevent dangling pointers to possibly freed memory. These pointers are
dereferenced in single_step_dabr_instruction() without a way to check
their validity.

This patch moves the tracking of this information to the breakpoint
itself. This means we no longer have to do anything special to clean up.

Signed-off-by: Benjamin Gray <bgray@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://msgid.link/20230801011744.153973-4-bgray@linux.ibm.com

authored by

Benjamin Gray and committed by
Michael Ellerman
1e60f356 668a6ec6

+23 -52
+1
arch/powerpc/include/asm/hw_breakpoint.h
··· 18 18 u16 len; /* length of the target data symbol */ 19 19 u16 hw_len; /* length programmed in hw */ 20 20 u8 flags; 21 + bool perf_single_step; /* temporarily uninstalled for a perf single step */ 21 22 }; 22 23 23 24 /* Note: Don't change the first 6 bits below as they are in the same order
-5
arch/powerpc/include/asm/processor.h
··· 172 172 unsigned int align_ctl; /* alignment handling control */ 173 173 #ifdef CONFIG_HAVE_HW_BREAKPOINT 174 174 struct perf_event *ptrace_bps[HBP_NUM_MAX]; 175 - /* 176 - * Helps identify source of single-step exception and subsequent 177 - * hw-breakpoint enablement 178 - */ 179 - struct perf_event *last_hit_ubp[HBP_NUM_MAX]; 180 175 #endif /* CONFIG_HAVE_HW_BREAKPOINT */ 181 176 struct arch_hw_breakpoint hw_brk[HBP_NUM_MAX]; /* hardware breakpoint info */ 182 177 unsigned long trap_nr; /* last trap # on this thread */
+22 -47
arch/powerpc/kernel/hw_breakpoint.c
··· 43 43 return 0; /* no instruction breakpoints available */ 44 44 } 45 45 46 - static bool single_step_pending(void) 47 - { 48 - int i; 49 - 50 - for (i = 0; i < nr_wp_slots(); i++) { 51 - if (current->thread.last_hit_ubp[i]) 52 - return true; 53 - } 54 - return false; 55 - } 56 46 57 47 /* 58 48 * Install a perf counter breakpoint. ··· 74 84 * Do not install DABR values if the instruction must be single-stepped. 75 85 * If so, DABR will be populated in single_step_dabr_instruction(). 76 86 */ 77 - if (!single_step_pending()) 87 + if (!info->perf_single_step) 78 88 __set_breakpoint(i, info); 79 89 80 90 return 0; ··· 362 372 } 363 373 364 374 /* 365 - * Perform cleanup of arch-specific counters during unregistration 366 - * of the perf-event 367 - */ 368 - void arch_unregister_hw_breakpoint(struct perf_event *bp) 369 - { 370 - /* 371 - * If the breakpoint is unregistered between a hw_breakpoint_handler() 372 - * and the single_step_dabr_instruction(), then cleanup the breakpoint 373 - * restoration variables to prevent dangling pointers. 374 - * FIXME, this should not be using bp->ctx at all! Sayeth peterz. 375 - */ 376 - if (bp->ctx && bp->ctx->task && bp->ctx->task != ((void *)-1L)) { 377 - int i; 378 - 379 - for (i = 0; i < nr_wp_slots(); i++) { 380 - if (bp->ctx->task->thread.last_hit_ubp[i] == bp) 381 - bp->ctx->task->thread.last_hit_ubp[i] = NULL; 382 - } 383 - } 384 - } 385 - 386 - /* 387 375 * Check for virtual address in kernel space. 388 376 */ 389 377 int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw) ··· 478 510 int i; 479 511 480 512 for (i = 0; i < nr_wp_slots(); i++) { 481 - if (unlikely(tsk->thread.last_hit_ubp[i])) 513 + struct perf_event *bp = __this_cpu_read(bp_per_reg[i]); 514 + 515 + if (unlikely(bp && counter_arch_bp(bp)->perf_single_step)) 482 516 goto reset; 483 517 } 484 518 return; ··· 490 520 for (i = 0; i < nr_wp_slots(); i++) { 491 521 info = counter_arch_bp(__this_cpu_read(bp_per_reg[i])); 492 522 __set_breakpoint(i, info); 493 - tsk->thread.last_hit_ubp[i] = NULL; 523 + info->perf_single_step = false; 494 524 } 495 525 } 496 526 ··· 533 563 for (i = 0; i < nr_wp_slots(); i++) { 534 564 if (!hit[i]) 535 565 continue; 536 - current->thread.last_hit_ubp[i] = bp[i]; 566 + 567 + counter_arch_bp(bp[i])->perf_single_step = true; 537 568 bp[i] = NULL; 538 569 } 539 570 regs_set_return_msr(regs, regs->msr | MSR_SE); ··· 741 770 static int single_step_dabr_instruction(struct die_args *args) 742 771 { 743 772 struct pt_regs *regs = args->regs; 744 - struct perf_event *bp = NULL; 745 - struct arch_hw_breakpoint *info; 746 - int i; 747 773 bool found = false; 748 774 749 775 /* 750 776 * Check if we are single-stepping as a result of a 751 777 * previous HW Breakpoint exception 752 778 */ 753 - for (i = 0; i < nr_wp_slots(); i++) { 754 - bp = current->thread.last_hit_ubp[i]; 779 + for (int i = 0; i < nr_wp_slots(); i++) { 780 + struct perf_event *bp; 781 + struct arch_hw_breakpoint *info; 782 + 783 + bp = __this_cpu_read(bp_per_reg[i]); 755 784 756 785 if (!bp) 757 786 continue; 758 787 759 - found = true; 760 788 info = counter_arch_bp(bp); 789 + 790 + if (!info->perf_single_step) 791 + continue; 792 + 793 + found = true; 761 794 762 795 /* 763 796 * We shall invoke the user-defined callback function in the ··· 770 795 */ 771 796 if (!(info->type & HW_BRK_TYPE_EXTRANEOUS_IRQ)) 772 797 perf_bp_event(bp, regs); 773 - current->thread.last_hit_ubp[i] = NULL; 798 + 799 + info->perf_single_step = false; 774 800 } 775 801 776 802 if (!found) 777 803 return NOTIFY_DONE; 778 804 779 - for (i = 0; i < nr_wp_slots(); i++) { 780 - bp = __this_cpu_read(bp_per_reg[i]); 805 + for (int i = 0; i < nr_wp_slots(); i++) { 806 + struct perf_event *bp = __this_cpu_read(bp_per_reg[i]); 781 807 if (!bp) 782 808 continue; 783 809 784 - info = counter_arch_bp(bp); 785 - __set_breakpoint(i, info); 810 + __set_breakpoint(i, counter_arch_bp(bp)); 786 811 } 787 812 788 813 /*