| Message ID | 154753353370.31541.14485875717131836689.stgit@devbox (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: kprobes: Update blacklist checking on arm64 | expand |
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) >
On Mon, 21 Jan 2019 12:20:07 +0000 James Morse <james.morse@arm.com> wrote: > 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. Yes, it is easy to explain how we transcribe from arch_within_kprobe_blacklist() to arch_populate_kprobe_blacklist(). > > > > - 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? Ah, good catch! No, we don't need it here. Sorry I worked on older patch. I'll update it. > 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? yes, so it should not. > > > > + 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. OK. > > 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. Hmm, I'm not sure when the original code decided this. But it sounds reasonable. > > 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. OK, then I should wait for that, because this series is a kind of improvement. But your's is a bugfix, that should be backported to stable. Thank you, > > > 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) > > >
Hi Masami, On Mon, Jan 21, 2019 at 10:25:58PM +0900, Masami Hiramatsu wrote: > On Mon, 21 Jan 2019 12:20:07 +0000 > James Morse <james.morse@arm.com> wrote: > > 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. > > Yes, it is easy to explain how we transcribe from > arch_within_kprobe_blacklist() to arch_populate_kprobe_blacklist(). > > > > > > > > - 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? > > Ah, good catch! No, we don't need it here. Sorry I worked on older patch. > I'll update it. Did you send a new version of this series? I can't seem to spot it in my inbox. Cheers, Will
On Fri, 8 Feb 2019 09:15:19 +0000 Will Deacon <will.deacon@arm.com> wrote: > Hi Masami, > > On Mon, Jan 21, 2019 at 10:25:58PM +0900, Masami Hiramatsu wrote: > > On Mon, 21 Jan 2019 12:20:07 +0000 > > James Morse <james.morse@arm.com> wrote: > > > 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. > > > > Yes, it is easy to explain how we transcribe from > > arch_within_kprobe_blacklist() to arch_populate_kprobe_blacklist(). > > > > > > > > > > > > - 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? > > > > Ah, good catch! No, we don't need it here. Sorry I worked on older patch. > > I'll update it. > > Did you send a new version of this series? I can't seem to spot it in my > inbox. Ah, OK. I just waited for James' patch series, https://patchwork.kernel.org/cover/10779489/ Are those merged? I'd like to move this series on that. Thank you for ping! :)
[+Marc] On Mon, Feb 11, 2019 at 10:10:23PM +0900, Masami Hiramatsu wrote: > On Fri, 8 Feb 2019 09:15:19 +0000 > Will Deacon <will.deacon@arm.com> wrote: > > Did you send a new version of this series? I can't seem to spot it in my > > inbox. > > Ah, OK. I just waited for James' patch series, > > https://patchwork.kernel.org/cover/10779489/ > > Are those merged? I'd like to move this series on that. Patches 2-4 are in mainline: f7daa9c8fd19 arm64: hibernate: Clean the __hyp_text to PoC after resume 8fac5cbdfe0f arm64: hyp-stub: Forbid kprobing of the hyp-stub f2b3d8566d81 arm64: kprobe: Always blacklist the KVM world-switch code Patch 1 is queued via kvm-arm (also for 5.0) but it doesn't seem to have landed yet. Will
On 11/02/2019 15:58, Will Deacon wrote: > [+Marc] > > On Mon, Feb 11, 2019 at 10:10:23PM +0900, Masami Hiramatsu wrote: >> On Fri, 8 Feb 2019 09:15:19 +0000 >> Will Deacon <will.deacon@arm.com> wrote: >>> Did you send a new version of this series? I can't seem to spot it in my >>> inbox. >> >> Ah, OK. I just waited for James' patch series, >> >> https://patchwork.kernel.org/cover/10779489/ >> >> Are those merged? I'd like to move this series on that. > > Patches 2-4 are in mainline: > > f7daa9c8fd19 arm64: hibernate: Clean the __hyp_text to PoC after resume > 8fac5cbdfe0f arm64: hyp-stub: Forbid kprobing of the hyp-stub > f2b3d8566d81 arm64: kprobe: Always blacklist the KVM world-switch code > > Patch 1 is queued via kvm-arm (also for 5.0) but it doesn't seem to have > landed yet. It was part of the pull request sent on Thursday[1], but Paolo hasn't pulled it yet. Hopefully soon... M. [1] https://patchwork.kernel.org/patch/10801151/
On Mon, 11 Feb 2019 16:05:17 +0000 Marc Zyngier <marc.zyngier@arm.com> wrote: > On 11/02/2019 15:58, Will Deacon wrote: > > [+Marc] > > > > On Mon, Feb 11, 2019 at 10:10:23PM +0900, Masami Hiramatsu wrote: > >> On Fri, 8 Feb 2019 09:15:19 +0000 > >> Will Deacon <will.deacon@arm.com> wrote: > >>> Did you send a new version of this series? I can't seem to spot it in my > >>> inbox. > >> > >> Ah, OK. I just waited for James' patch series, > >> > >> https://patchwork.kernel.org/cover/10779489/ > >> > >> Are those merged? I'd like to move this series on that. > > > > Patches 2-4 are in mainline: > > > > f7daa9c8fd19 arm64: hibernate: Clean the __hyp_text to PoC after resume > > 8fac5cbdfe0f arm64: hyp-stub: Forbid kprobing of the hyp-stub > > f2b3d8566d81 arm64: kprobe: Always blacklist the KVM world-switch code > > > > Patch 1 is queued via kvm-arm (also for 5.0) but it doesn't seem to have > > landed yet. > > It was part of the pull request sent on Thursday[1], but Paolo hasn't > pulled it yet. > > Hopefully soon... OK, then I'll send updated series since Patch1 is independent from this series. Thank you,
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)) - 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; + 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; + + 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)
Use arch_populate_kprobe_blacklist() instead of arch_within_kprobe_blacklist() so that we can see the full blacklisted symbols under the debugfs. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> --- arch/arm64/kernel/probes/kprobes.c | 42 ++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 19 deletions(-)