Hello,
On 15/01/2019 06:25, Masami Hiramatsu wrote:
> Use arch_populate_kprobe_blacklist() instead of
> arch_within_kprobe_blacklist() so that we can see the full
> blacklisted symbols under the debugfs.
> diff --git a/arch/arm64/kernel/probes/kprobes.c
> b/arch/arm64/kernel/probes/kprobes.c
> index b9e9758b6534..6c066c34c8a4 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -465,26 +465,30 @@ kprobe_breakpoint_handler(struct pt_regs *regs,
> unsigned int esr)
> return DBG_HOOK_HANDLED;
> }
>
> -bool arch_within_kprobe_blacklist(unsigned long addr)
> +int __init arch_populate_kprobe_blacklist(void)
> {
> - if ((addr >= (unsigned long)__kprobes_text_start &&
> - addr < (unsigned long)__kprobes_text_end) ||
> - (addr >= (unsigned long)__entry_text_start &&
> - addr < (unsigned long)__entry_text_end) ||
> - (addr >= (unsigned long)__idmap_text_start &&
> - addr < (unsigned long)__idmap_text_end) ||
> - in_exception_text(addr))
You added this one in the previous patch, but it disappears here.
> - return true;
> -
> - if (!is_kernel_in_hyp_mode()) {
> - if ((addr >= (unsigned long)__hyp_text_start &&
> - addr < (unsigned long)__hyp_text_end) ||
> - (addr >= (unsigned long)__hyp_idmap_text_start &&
> - addr < (unsigned long)__hyp_idmap_text_end))
> - return true;
> - }
> -
> - return false;
> + int ret;
> + ret = kprobe_add_area_blacklist((unsigned long)__kprobes_text_start,
> + (unsigned long)__kprobes_text_end);
> + if (ret)
> + return ret;
Now that we have arch_populate_kprobe_blacklist(), does the arch-code need to
blacklist the kprobes section itself?
The weak arch_within_kprobe_blacklist() will test it at kprobe-load time, and
populate_kprobe_blacklist() adds it to the list before it calls
arch_populate_kprobe_blacklist().
Won't this result in duplicate entries?
> + ret = kprobe_add_area_blacklist((unsigned long)__entry_text_start,
> + (unsigned long)__entry_text_end);
> + if (ret)
> + return ret;
> + ret = kprobe_add_area_blacklist((unsigned long)__idmap_text_start,
> + (unsigned long)__idmap_text_end);
> + if (ret || is_kernel_in_hyp_mode())
> + return ret;
Hmmm, I think we have a bug here today.
This is saying we can kprobe KVM when we have VHE, because all of KVMs code runs
at the same exception-level as the kernel. Which is true...
But KVM switches VBAR_EL1, so if we run over one of kprobes BRK instructions,
we're going to hyp-panic, because KVM doesn't handle synchronous exceptions from
EL2.
The __hyp_text also contains the guest entry/exit code, which we mustn't probe,
even on VHE.
I think we should always blacklist the __hyp_text, and KVM should mark its
vhe-only functions with __kprobes. I'll post patches for this.
Thanks,
James
> + ret = kprobe_add_area_blacklist((unsigned long)__hyp_text_start,
> + (unsigned long)__hyp_text_end);
> + if (ret)
> + return ret;
> + ret = kprobe_add_area_blacklist((unsigned long)__hyp_idmap_text_start,
> + (unsigned long)__hyp_idmap_text_end);
> + return ret;
> }
>
> void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
>
