On Fri, Jun 05, 2020 at 04:44:57PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:
> > On Fri, Jun 05, 2020 at 02:21:26PM +0100, Daniel Thompson wrote:
> > > kgdb has traditionally adopted a no safety rails approach to breakpoint
> > > placement. If the debugger is commanded to place a breakpoint at an
> > > address then it will do so even if that breakpoint results in kgdb
> > > becoming inoperable.
> > >
> > > A stop-the-world debugger with memory peek/poke does intrinsically
> > > provide its operator with the means to hose their system in all manner
> > > of exciting ways (not least because stopping-the-world is already a DoS
> > > attack ;-) ) but the current no safety rail approach is not easy to
> > > defend, especially given kprobes provides us with plenty of machinery to
> > > mark parts of the kernel where breakpointing is discouraged.
> > >
> > > This patchset introduces some safety rails by using the existing
> > > kprobes infrastructure. It does not cover all locations where
> > > breakpoints can cause trouble but it will definitely block off several
> > > avenues, including the architecture specific parts that are handled by
> > > arch_within_kprobe_blacklist().
> > >
> > > This patch is an RFC because:
> > >
> > > 1. My workstation is still chugging through the compile testing.
> > >
> > > 2. Patch 4 needs more runtime testing.
> > >
> > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> > > more review especially for its impact on arch specific code.
> > >
> > > To be clear I do plan to do the detailed review of the kprobe blacklist
> > > stuff but would like to check the direction of travel first since the
> > > change is already surprisingly big and maybe there's a better way to
> > > organise things.
> >
> > Thanks for doing these patches, esp 1-3 look very good to me.
> >
> > I've taken the liberty to bounce the entire set to Masami-San, who is
> > the kprobes maintainer for comments as well.
>
> OK, after having had a second look, one thing we can perhaps address
> with the last patch, or perhaps on top of that, is extending the
> kprobes_blacklist() with data regions.
>
> Because these patches only exclude kgdb from setting breakpoints on
> code; data breakpoints do not match what we do with
> arch_build_bp_info().
Right, my patches will only affect the code paths where kgdb sets
software breakpoints.
In principle h/w breakpoints, whether they are code or data, should be
able to rely on hw_breakpoint_arch_parse() and any errors from the hw
breakpoint API will propagate into the kgdb core and do the right
thing.
In practice it looks like kgdb/x86/hw_breakpoint have plumbed together
in a manner that circumvents the parse (and therefore#
arch_build_bp_info() never runs). Rather the h/w/ break problem using
the kprobe blacklist I think we could just fix these code paths.
The following is 100% untested (not even compile) and I copied a line
or two from register_perf_hw_breakpoint() without checking what they
do ;-). Nevertheless I hope it gives a clear idea about what I am
talking about! If this was developed into a "real" patch then I think
dbg_release_bp_slot() should perhaps be replaced with a better API that
doesn't bypass the checks rather than solving everything in kgdb.c .
Daniel.
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index c44fe7d8d9a4..64ac0ee9b55c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -223,11 +223,12 @@ static void kgdb_correct_hw_break(void)
hw_breakpoint_restore();
}
-static int hw_break_reserve_slot(int breakno)
+static int kgdb_register_hw_break(int breakno)
{
int cpu;
int cnt = 0;
struct perf_event **pevent;
+ struct arch_hw_breakpoint hw = { };
if (dbg_is_early)
return 0;
@@ -237,6 +238,11 @@ static int hw_break_reserve_slot(int breakno)
pevent = per_cpu_ptr(breakinfo[breakno].pev, cpu);
if (dbg_reserve_bp_slot(*pevent))
goto fail;
+ if (hw_breakpoint_arch_parse(*pevent, &(*pevent)->attr, hw)) {
+ cnt++;
+ goto fail;
+ }
+ (*pevent)->hw.info = hw;
}
return 0;
@@ -361,7 +367,7 @@ kgdb_set_hw_break(unsigned long addr, int len, enum
kgdb_bptype bptype)
return -1;
}
breakinfo[i].addr = addr;
- if (hw_break_reserve_slot(i)) {
+ if (kgdb_register_hw_break(i)) {
breakinfo[i].addr = 0;
return -1;
}
