Re: [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup

[Ingo Molnar <mingo@kernel.org>]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> This series fixes a kprobe-x86 bug related to RO text and
> cleans up addressof operators.
> 
> The first one is an obvious bug that misses to set memory
> RO when the function fails. And the second one is just a
> cleanup patch to remove addressof operators ("&") since
> it is meaningless anymore.
> 
> V2 has just a following update:
>  - [1/2] Use a helper variable instead of using p->ainsn.insn
>    directly.
> 
> Thanks,
> 
> ---
> 
> Masami Hiramatsu (2):
>       kprobes/x86: Don't forget to set memory back to RO on failure
>       kprobes/x86: Remove addressof operators
> 
> 
>  arch/x86/include/asm/kprobes.h |    4 ++--
>  arch/x86/kernel/kprobes/core.c |   15 +++++++++------
>  arch/x86/kernel/kprobes/opt.c  |    9 +++++----
>  3 files changed, 16 insertions(+), 12 deletions(-)

So I'm totally opposed to the whole approach of modifying the permissions of the 
kernel text virtual memory area.

Firstly, it's racy against other kernel subsystems: what happens if some other 
code patching mechanism does a similar 'mark RWX and then back to RX'? Who 
provides the synchronization? set_memory_*() certainly does not.

Secondly, it's racy against attackers: if an attacker can time the attack to the 
time when a kprobe is installed, then the kernel is still vulnerable.

So how about avoiding the problem altogether by patching the kernel not in its 
virtual text address, but in the direct mappings? Then page permissions won't have 
to be modified, and the whole solution will be more robust and secure.

Is there anything I'm missing?

Thanks,

	Ingo