linux-kernel - Re: [PATCH] kgdb: Fix to kill breakpoints on initmem after boot

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=VOdf1TyXWQOyP=y2xaLxY6_c+xm-VSSUkFasJD1Cbgpw@mail.gmail.com>
Date:   Wed, 24 Feb 2021 10:09:25 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Sumit Garg <sumit.garg@...aro.org>
Cc:     kgdb-bugreport@...ts.sourceforge.net,
        Andrew Morton <akpm@...ux-foundation.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jason Wessel <jason.wessel@...driver.com>,
        Peter Zijlstra <peterz@...radead.org>, stefan.saecherl@....de,
        qy15sije@....cs.fau.de, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kgdb: Fix to kill breakpoints on initmem after boot

Hi,

On Wed, Feb 24, 2021 at 12:17 AM Sumit Garg <sumit.garg@...aro.org> wrote:
>
> Currently breakpoints in kernel .init.text section are not handled
> correctly while allowing to remove them even after corresponding pages
> have been freed.
>
> Fix it via killing .init.text section breakpoints just prior to initmem
> pages being freed.

It might be worth it to mention that HW breakpoints aren't handled by
this patch but it's probably not such a big deal.


> Suggested-by: Doug Anderson <dianders@...omium.org>
> Signed-off-by: Sumit Garg <sumit.garg@...aro.org>
> ---
>  include/linux/kgdb.h      |  2 ++
>  init/main.c               |  1 +
>  kernel/debug/debug_core.c | 11 +++++++++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
> index 57b8885708e5..3aa503ef06fc 100644
> --- a/include/linux/kgdb.h
> +++ b/include/linux/kgdb.h
> @@ -361,9 +361,11 @@ extern atomic_t                    kgdb_active;
>  extern bool dbg_is_early;
>  extern void __init dbg_late_init(void);
>  extern void kgdb_panic(const char *msg);
> +extern void kgdb_free_init_mem(void);
>  #else /* ! CONFIG_KGDB */
>  #define in_dbg_master() (0)
>  #define dbg_late_init()
>  static inline void kgdb_panic(const char *msg) {}
> +static inline void kgdb_free_init_mem(void) { }
>  #endif /* ! CONFIG_KGDB */
>  #endif /* _KGDB_H_ */
> diff --git a/init/main.c b/init/main.c
> index c68d784376ca..a446ca3d334e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1417,6 +1417,7 @@ static int __ref kernel_init(void *unused)
>         async_synchronize_full();
>         kprobe_free_init_mem();
>         ftrace_free_init_mem();
> +       kgdb_free_init_mem();
>         free_initmem();
>         mark_readonly();
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 229dd119f430..319381e95d1d 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -465,6 +465,17 @@ int dbg_remove_all_break(void)
>         return 0;
>  }
>
> +void kgdb_free_init_mem(void)
> +{
> +       int i;
> +
> +       /* Clear init memory breakpoints. */
> +       for (i = 0; i < KGDB_MAX_BREAKPOINTS; i++) {
> +               if (init_section_contains((void *)kgdb_break[i].bpt_addr, 0))

A nit, but instead of 0 should this be passing "BREAK_INSTR_SIZE" ?

Also: even if memory is about to get freed it still seems like it'd be
wise to call this:

  kgdb_arch_remove_breakpoint(&kgdb_break[i]);

It looks like it shouldn't matter today but just in case an
architecture decides to do something fancy in the future it might not
hurt to tell it that the breakpoint is going away.


Everything here is pretty nitty, though.  This looks good to me now.

Reviewed-by: Douglas Anderson <dianders@...omium.org>