Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault

[Linus Torvalds]

On Sun, Feb 24, 2019 at 7:18 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Sat, 23 Feb 2019 20:38:03 -0800
> Andy Lutomirski <luto@xxxxxxxxxx> wrote:
> >
> > Can we just get rid of this might_sleep()?  access_ok() doesn't sleep
> > as far as I know.
>
> Hmm, which might_sleep() would you pointed? What I talked was a
> WARN_ON_ONCE(!in_task()) in access_ok() on x86 (only!), and in_task() just
> checks preempt_count.

So the in_task() check does kind of make sense. Using "access_ok()"
outside of task context is certainly an odd thing, for several
reasons. The main one being simply that outside of task context, the
whole "which task" question is open, and you don't know if the task is
the active one, and so it's not clear if whatever task you interrupt
might have done "set_fs()" or not.

So PeterZ isn't wrong:

> I guess PeterZ assumed that access_ok() is used only with user space access
> APIs (e.g. copy_from_user) which can cause page-fault and locks mm (and might
> sleep :)), but now we are trying to use access_ok() with new functions which
> disables page-fault and just return -EFAULT.

.. but in this case, if we do it all *within* code that saves and
restores the user access flag with get_fs/set_fs, access_ok() would be
ok and it doesn't have the above issue.

So access_ok() in _general_ is absolutely not safe to do from
interrupts, but within the context of probing user memory from a
tracing event it just happens to be ok.

It would be lovely to have a special macro for this, and keep the
warning for the general case, but because this is a "every
architecture needs to build their own" it's probably too painful.

PeterZ, do you remember the particular use case that triggered that
commit 7c4788950ba5 ("x86/uaccess, sched/preempt: Verify access_ok()
context")?

                    Linus