Messages in this thread | | | From | Andy Lutomirski <> | Date | Thu, 22 Sep 2016 15:44:37 -0700 | Subject | Re: [PATCH 09/12] x86/process: Pin the target stack in get_wchan() |
| |
On Fri, Sep 16, 2016 at 7:00 PM, Jann Horn <jann@thejh.net> wrote: > On Tue, Sep 13, 2016 at 02:29:29PM -0700, Andy Lutomirski wrote: >> This will prevent a crash if get_wchan() runs after the task stack >> is freed. > > I think I found some more stuff. Have a look at KSTK_EIP() and KSTK_ESP(), I think > they read from the saved userspace registers area at the top of the kernel stack? > > Used on remote processes in: > vma_is_stack_for_task() (via /proc/$pid/maps)
This isn't used in /proc/$pid/maps -- it's only used in /proc/$pid/task/$tid/maps. I wonder if anyone actually cares about it -- it certainly won't work reliably.
I could pin the stack in vma_is_stack_for_task, but it seems potentially better to me to change it to vma_is_stack_for_current() and remove the offending caller in /proc, replacing it with "return 0". Thoughts?
> do_task_stat() (/proc/$pid/stat)
Like this:
mm = get_task_mm(task); if (mm) { vsize = task_vsize(mm); if (permitted) { eip = KSTK_EIP(task); esp = KSTK_ESP(task); } }
Can we just delete this outright? It seems somewhere between mostly and entirely useless, and it also seems dangerous. Until very recently, on x86_64, this would have been a potential info leak, as SYSCALL followed closely by a hardware interrupt would cause *kernel* values to land in task_pt_regs(). I don't even want to think about what this code does if the task is in vm86 mode. I wouldn't be at all surprised if non-x86 architectures have all kinds of interesting thinks happen if you do this to a task that isn't running normal non-atomic kernel code at the time.
I would advocate for unconditionally returning zeros in these two stat fields.
| |