Russell Currey wrote:
On Fri, 2020-04-03 at 15:06 +0530, Naveen N. Rao wrote:
Russell Currey wrote:
> On Fri, 2020-04-03 at 00:18 +0530, Naveen N. Rao wrote:
> > Naveen N. Rao wrote:
> > > Russell Currey wrote:
> > > >
> > > > +void *alloc_insn_page(void)
> > > > +{
> > > > + void *page = vmalloc_exec(PAGE_SIZE);
> > > > +
> > > > + if (page)
> > > > + set_memory_ro((unsigned long)page, 1);
> > > > +
> > > > + return page;
> > > > +}
> > > > +
> > >
> > > This crashes for me with KPROBES_SANITY_TEST during the
> > > kretprobe
> > > test.
> >
> > That isn't needed to reproduce this. After bootup, disabling
> > optprobes
> > also shows the crash with kretprobes:
> > sysctl debug.kprobes-optimization=0
> >
> > The problem happens to be with patch_instruction() in
> > arch_prepare_kprobe(). During boot, on kprobe init, we register a
> > probe
> > on kretprobe_trampoline for use with kretprobes (see
> > arch_init_kprobes()). This results in an instruction slot being
> > allocated, and arch_prepare_kprobe() to be called for copying
> > the
> > instruction (nop) at kretprobe_trampoline. patch_instruction()
> > is
> > failing resulting in corrupt instruction which we try to
> > emulate/single
> > step causing the crash.
>
> OK I think I've fixed it, KPROBES_SANITY_TEST passes too. I'd
> appreciate it if you could test v9, and thanks again for finding
> this -
> very embarrassing bug on my side.
Great! Thanks.
I think I should also add appropriate error checking to kprobes' use
of
patch_instruction() which would have caught this much more easily.
Only kind of! It turns out that if the initial setup fails for
KPROBES_SANITY_TEST, it silently doesn't run - so you miss the "Kprobe
smoke test" text, but you don't get any kind of error either. I'll
send a patch so that it fails more loudly later.
Ha, I see what you mean. Good catch, we should pass the kprobe init
status to the test and have it error out.
On a related note, I notice that x86 seems to prefer not having any
RWX
pages, and so they continue to do 'module_alloc()' followed by
'set_memory_ro()' and then 'set_memory_x()'. Is that something worth
following for powerpc?
I just noticed that too. arm64 doesn't set theirs executable, as far
as I can tell powerpc doesn't need to.
I didn't follow that. We do need it to be executable so that we can
single step the original instruction.
arm64 does vmalloc_exec(), which looks like it sets the page to RWX,
then marks it RO. There is a small window where the page would be WX.
x86 instead seems to first allocate the page as RW, mark as RO, and only
then enable X - removing that small window where the page is both W and
X.
- Naveen
