| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201218164421.qy5ov2lyhrfi3kr6@holly.lan>
Date: Fri, 18 Dec 2020 16:44:21 +0000
From: Daniel Thompson <daniel.thompson@...aro.org>
To: Stefan Saecherl <stefan.saecherl@....de>
Cc: x86@...nel.org, linux-kernel@...cs.fau.de,
Lorena Kretzschmar <qy15sije@....cs.fau.de>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
"H. Peter Anvin" <hpa@...or.com>,
Peter Zijlstra <peterz@...radead.org>,
Alexandre Chartre <alexandre.chartre@...cle.com>,
Mike Rapoport <rppt@...nel.org>,
Ira Weiny <ira.weiny@...el.com>,
Adrian Hunter <adrian.hunter@...el.com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/kgdb: Allow removal of early BPs
Hi Stefan
On Mon, Dec 14, 2020 at 03:13:12PM +0100, Stefan Saecherl wrote:
> The problem is that breakpoints that are set early (e.g. via kgdbwait)
> cannot be deleted after boot completed (to be precise after mark_rodata_ro
> ran).
>
> When setting a breakpoint early there are executable pages that are
> writable so the copy_to_kernel_nofault call in kgdb_arch_set_breakpoint
> succeeds and the breakpoint is saved as type BP_BREAKPOINT.
>
> Later in the boot write access to these pages is restricted. So when
> removing the breakpoint the copy_to_kernel_nofault call in
> kgdb_arch_remove_breakpoint is destined to fail and the breakpoint removal
> fails. So after copy_to_kernel_nofault failed try to text_poke_kgdb which
> can work around nonwriteability.
>
> One thing to consider when doing this is that code can go away during boot
> (e.g. .init.text). Previously kgdb_arch_remove_breakpoint handled this case
> gracefully by just having copy_to_kernel_nofault fail but if one then calls
> text_poke_kgdb the system dies due to the BUG_ON we moved out of
> __text_poke. To avoid this __text_poke now returns an error in case of a
> nonpresent code page and the error is handled at call site.
>
> Checkpatch complains about two uses of BUG_ON but the new code should not
> trigger BUG_ON in cases where the old didn't.
>
> Co-developed-by: Lorena Kretzschmar <qy15sije@....cs.fau.de>
> Signed-off-by: Lorena Kretzschmar <qy15sije@....cs.fau.de>
> Signed-off-by: Stefan Saecherl <stefan.saecherl@....de>
I took this to be a gap in the kgdbtest suite so I added a couple of
tests that cover this area. Before this patch they failed now they
pass (at least they do for ARCH=x86).
I don't see any new failures either, so:
Tested-by: Daniel Thompson <daniel.thompson@...aro.org>
Daniel.
> ---
> arch/x86/kernel/alternative.c | 16 +++++++----
> arch/x86/kernel/kgdb.c | 54 ++++++++++++++++++++++++-----------
> 2 files changed, 48 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 2400ad62f330..0f145d837885 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -878,11 +878,9 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
> if (cross_page_boundary)
> pages[1] = virt_to_page(addr + PAGE_SIZE);
> }
> - /*
> - * If something went wrong, crash and burn since recovery paths are not
> - * implemented.
> - */
> - BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
> +
> + if (!pages[0] || (cross_page_boundary && !pages[1]))
> + return ERR_PTR(-EFAULT);
>
> /*
> * Map the page without the global bit, as TLB flushing is done with
> @@ -976,7 +974,13 @@ void *text_poke(void *addr, const void *opcode, size_t len)
> {
> lockdep_assert_held(&text_mutex);
>
> - return __text_poke(addr, opcode, len);
> + addr = __text_poke(addr, opcode, len);
> + /*
> + * If something went wrong, crash and burn since recovery paths are not
> + * implemented.
> + */
> + BUG_ON(IS_ERR(addr));
> + return addr;
> }
>
> /**
> diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
> index ff7878df96b4..e98c9c43db7c 100644
> --- a/arch/x86/kernel/kgdb.c
> +++ b/arch/x86/kernel/kgdb.c
> @@ -731,6 +731,7 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long ip)
> int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> {
> int err;
> + void *addr;
>
> bpt->type = BP_BREAKPOINT;
> err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
> @@ -747,8 +748,14 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
> */
> if (mutex_is_locked(&text_mutex))
> return -EBUSY;
> - text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
> - BREAK_INSTR_SIZE);
> +
> + addr = text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
> + BREAK_INSTR_SIZE);
> + /* This should never trigger because the above call to copy_from_kernel_nofault
> + * already succeeded.
> + */
> + BUG_ON(IS_ERR(addr));
> +
> bpt->type = BP_POKE_BREAKPOINT;
>
> return 0;
> @@ -756,21 +763,36 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
>
> int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
> {
> - if (bpt->type != BP_POKE_BREAKPOINT)
> - goto knl_write;
> - /*
> - * It is safe to call text_poke_kgdb() because normal kernel execution
> - * is stopped on all cores, so long as the text_mutex is not locked.
> - */
> - if (mutex_is_locked(&text_mutex))
> - goto knl_write;
> - text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
> - BREAK_INSTR_SIZE);
> - return 0;
> + void *addr;
> + int err;
>
> -knl_write:
> - return copy_to_kernel_nofault((char *)bpt->bpt_addr,
> - (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
> + if (bpt->type == BP_POKE_BREAKPOINT) {
> + if (mutex_is_locked(&text_mutex)) {
> + err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
> + (char *)bpt->saved_instr,
> + BREAK_INSTR_SIZE);
> + } else {
> + /*
> + * It is safe to call text_poke_kgdb() because normal kernel execution
> + * is stopped on all cores, so long as the text_mutex is not locked.
> + */
> + addr = text_poke_kgdb((void *)bpt->bpt_addr,
> + bpt->saved_instr,
> + BREAK_INSTR_SIZE);
> + err = PTR_ERR_OR_ZERO(addr);
> + }
> + } else {
> + err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
> + (char *)bpt->saved_instr,
> + BREAK_INSTR_SIZE);
> + if (err == -EFAULT && !mutex_is_locked(&text_mutex)) {
> + addr = text_poke_kgdb((void *)bpt->bpt_addr,
> + bpt->saved_instr,
> + BREAK_INSTR_SIZE);
> + err = PTR_ERR_OR_ZERO(addr);
> + }
> + }
> + return err;
> }
>
> const struct kgdb_arch arch_kgdb_ops = {
> --
> 2.20.1
Powered by blists - more mailing lists