Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180830194736.210039-1-jannh@google.com> <20180831082712.cbfvv655amizvkfq@ltop.local>
In-Reply-To: <20180831082712.cbfvv655amizvkfq@ltop.local>
From:   Jann Horn <jannh@google.com>
Date:   Fri, 31 Aug 2018 15:26:24 +0200
Message-ID: <CAG48ez1aw=fSfDEp14=d+GxSu=3hLYP70Sd_s1Xux4MPiNM2VA@mail.gmail.com>
Subject: Re: [PATCH] x86/dumpstack: fix address space casting in show_opcodes()
To:     Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        kernel list <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>, bp@suse.de
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Aug 31, 2018 at 10:27 AM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Thu, Aug 30, 2018 at 09:47:36PM +0200, Jann Horn wrote:
> > I sloppily passed a kernel-typed pointer to __range_not_ok(), and sparse
> > doesn't like that.
> > Make `prologue` a __user pointer (to protect against accidental
> > dereferences) and force-cast it to a kernel pointer when calling
> > probe_kernel_read(), which will then immediately force-cast it back to a
> > user pointer.
>
> It's a bit sad to have to do this.
> __range_not_ok() explicitly requires a __user pointer (I don't know
> if there is a good reason for it) but  the real job is done by
> __chk_range_not_ok(). Can't you use this later instead?

Yeah, I guess I can do that. Will send a v2 in a bit...


By the way, here are all 60 probe_kernel_read() callers:

arch/arm/kernel/ftrace.c:               if
(probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
arch/arm/kernel/kgdb.c: err = probe_kernel_read(bpt->saved_instr,
(char *)bpt->bpt_addr,
arch/arm64/kernel/insn.c:       ret = probe_kernel_read(&val, addr,
AARCH64_INSN_SIZE);
arch/ia64/kernel/ftrace.c:      if (probe_kernel_read(replaced, (void
*)ip, MCOUNT_INSN_SIZE))
arch/ia64/kernel/ftrace.c:      if (probe_kernel_read(replaced, (void
*)ip, MCOUNT_INSN_SIZE))
arch/mips/kernel/kprobes.c:     if ((probe_kernel_read(&prev_insn, p->addr - 1,
arch/powerpc/kernel/module_64.c:        if (probe_kernel_read(&magic,
&stub->magic, sizeof(magic))) {
arch/powerpc/kernel/module_64.c:        if
(probe_kernel_read(&funcdata, &stub->funcdata, sizeof(funcdata))) {
arch/powerpc/kernel/trace/ftrace.c:     if
(probe_kernel_read(&replaced, (void *)ip, MCOUNT_INSN_SIZE))
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(&op,
(void *)ip, sizeof(int))) {
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(&op,
(void *)(ip - 4), 4)) {
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(&op,
(void *)(ip+4), MCOUNT_INSN_SIZE)) {
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(&op,
(void *)ip, MCOUNT_INSN_SIZE))
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(jmp,
(void *)tramp, sizeof(jmp))) {
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(op, ip,
sizeof(op)))
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(&op,
(void *)ip, MCOUNT_INSN_SIZE))
arch/powerpc/kernel/trace/ftrace.c:     if (probe_kernel_read(&op,
(void *)ip, sizeof(int))) {
arch/powerpc/perf/core-book3s.c:                if
(probe_kernel_read(&instr, (void *)addr, sizeof(instr)))
arch/riscv/kernel/ftrace.c:     if (probe_kernel_read(replaced, (void
*)hook_pos, MCOUNT_INSN_SIZE))
arch/s390/kernel/ftrace.c:      if (probe_kernel_read(&old, (void *)
rec->ip, sizeof(old)))
arch/s390/kernel/ftrace.c:      if (probe_kernel_read(&old, (void *)
rec->ip, sizeof(old)))
arch/sh/kernel/ftrace.c:        if (probe_kernel_read(replaced, (void
*)ip, MCOUNT_INSN_SIZE))
arch/sh/kernel/ftrace.c:        if (probe_kernel_read(code, (void
*)ip, MCOUNT_INSN_SIZE))
arch/x86/kernel/dumpstack.c:    if (probe_kernel_read(opcodes, rip -
PROLOGUE_SIZE, OPCODE_BUFSIZE)) {
arch/x86/kernel/ftrace.c:       if (probe_kernel_read(replaced, (void
*)ip, MCOUNT_INSN_SIZE))
arch/x86/kernel/ftrace.c:       if (probe_kernel_read(replaced, (void
*)ip, MCOUNT_INSN_SIZE))
arch/x86/kernel/ftrace.c:       if (probe_kernel_read(ins, (void *)ip,
MCOUNT_INSN_SIZE))
arch/x86/kernel/ftrace.c:       ret = probe_kernel_read(trampoline,
(void *)start_offset, size);
arch/x86/kernel/ftrace.c:       ret = probe_kernel_read(&calc, ptr,
MCOUNT_INSN_SIZE);
arch/x86/kernel/kgdb.c: err = probe_kernel_read(bpt->saved_instr,
(char *)bpt->bpt_addr,
arch/x86/kernel/kgdb.c: err = probe_kernel_read(opc, (char
*)bpt->bpt_addr, BREAK_INSTR_SIZE);
arch/x86/kernel/kgdb.c: err = probe_kernel_read(opc, (char
*)bpt->bpt_addr, BREAK_INSTR_SIZE);
arch/x86/kernel/kprobes/core.c: if (probe_kernel_read(buf, (void *)addr,
arch/x86/kernel/kprobes/core.c: if (probe_kernel_read(dest, (void
*)recovered_insn, MAX_INSN_SIZE))
arch/x86/kernel/kprobes/opt.c:  if (probe_kernel_read(buf, (void *)addr,
arch/x86/xen/enlighten_pv.c:    probe_kernel_read(&dummy, v, 1);
drivers/char/mem.c:                     probe =
probe_kernel_read(bounce, ptr, sz);
drivers/dio/dio.c:                if (probe_kernel_read(&i, (unsigned
char *)va + DIO_IDOFF, 1)) {
drivers/dio/dio.c:                if (probe_kernel_read(&i, (unsigned
char *)va + DIO_IDOFF, 1)) {
drivers/input/serio/hp_sdc.c:   if (!probe_kernel_read(&i, (unsigned
char *)hp_sdc.data_io, 1))
drivers/misc/kgdbts.c:  probe_kernel_read(before, (char *)kgdbts_break_test,
drivers/misc/kgdbts.c:  probe_kernel_read(after, (char *)kgdbts_break_test,
drivers/video/fbdev/hpfb.c:     err = probe_kernel_read(&i, (unsigned
char *)INTFBVADDR + DIO_IDOFF, 1);
fs/proc/kcore.c:                                if
(probe_kernel_read(buf, (void *) start, tsz)) {
include/linux/uaccess.h:        probe_kernel_read(&retval, addr, sizeof(retval))
kernel/debug/debug_core.c:      err =
probe_kernel_read(bpt->saved_instr, (char *)bpt->bpt_addr,
kernel/debug/gdbstub.c: err = probe_kernel_read(tmp, mem, count);
kernel/debug/kdb/kdb_main.c:    if (!p || probe_kernel_read(&tmp,
(char *)p, sizeof(unsigned long)))
kernel/debug/kdb/kdb_support.c: int ret = probe_kernel_read((char
*)res, (char *)addr, size);
kernel/debug/kdb/kdb_support.c: int ret = probe_kernel_read((char
*)addr, (char *)res, size);
kernel/debug/kdb/kdb_support.c: if (!p || probe_kernel_read(&tmp,
(char *)p, sizeof(unsigned long)))
kernel/kthread.c:       probe_kernel_read(&data, &kthread->data, sizeof(data));
kernel/trace/bpf_trace.c:       ret = probe_kernel_read(dst, unsafe_ptr, size);
kernel/workqueue.c:     probe_kernel_read(&fn, &worker->current_func,
sizeof(fn));
kernel/workqueue.c:     probe_kernel_read(&pwq, &worker->current_pwq,
sizeof(pwq));
kernel/workqueue.c:     probe_kernel_read(&wq, &pwq->wq, sizeof(wq));
kernel/workqueue.c:     probe_kernel_read(name, wq->name, sizeof(name) - 1);
kernel/workqueue.c:     probe_kernel_read(desc, worker->desc, sizeof(desc) - 1);
mm/slab.c:              if (probe_kernel_read(&v, dbg_userword(c, p),
sizeof(v)))
mm/slub.c:      probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));

41 of these (or something like that, I counted by hand) have some sort
of cast in the call expression.
probe_kernel_read() is kinda special in that expected types for the
second argument are both kernel pointers and unsigned longs. It might
make sense to have a wrapper macro around probe_kernel_read() that
accepts anything as long as it's as wide as a pointer... maybe
something for a future refactor.