linux-kernel - Re: [PATCH] kgdb: Resolve races during kgdb_io_register/unregister

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=W9rdEsO1jP-kg6OetXmZO+kC9LenZM=CdxjUvv8BEU4g@mail.gmail.com>
Date:   Mon, 29 Jun 2020 14:03:52 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        Petr Mladek <pmladek@...e.com>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>,
        Patch Tracking <patches@...aro.org>
Subject: Re: [PATCH] kgdb: Resolve races during kgdb_io_register/unregister_module

Hi,

On Mon, Jun 29, 2020 at 10:15 AM Daniel Thompson
<daniel.thompson@...aro.org> wrote:
>
> Currently kgdb_register_callbacks() and kgdb_unregister_callbacks()
> are called outside the scope of the kgdb_registration_lock. This
> allows them to race with each other. This could do all sorts of crazy
> things up to and including dbg_io_ops becoming NULL partway through the
> execution of the kgdb trap handler (which isn't allowed and would be
> fatal).
>
> Fix this by bringing the trap handler setup and teardown into the scope
> of the registration lock.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@...aro.org>
> ---
>  kernel/debug/debug_core.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 9e5934780f41..9799f2c6dc94 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -1117,9 +1117,8 @@ int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
>
>         dbg_io_ops = new_dbg_io_ops;
>
> -       spin_unlock(&kgdb_registration_lock);
> -
>         if (old_dbg_io_ops) {
> +               spin_unlock(&kgdb_registration_lock);
>                 old_dbg_io_ops->deinit();
>                 return 0;
>         }
> @@ -1129,6 +1128,8 @@ int kgdb_register_io_module(struct kgdb_io *new_dbg_io_ops)
>         /* Arm KGDB now. */
>         kgdb_register_callbacks();
>
> +       spin_unlock(&kgdb_registration_lock);

>From looking at code paths, I think this is illegal, isn't it?  You're
now calling kgdb_register_callbacks() while holding a spinlock, but:

kgdb_register_callbacks()
-> register_console()
--> console_lock()
---> might_sleep()
----> <boom!>


I'm a little curious about the exact race we're trying to solve.
Calling unregister on an IO module before register even finished seems
like an error on the caller, so I guess it would be calling register
from a 2nd thread for a different IO module while the first thread was
partway through unregistering?  Even that seems awfully sketchy since
you're risking registering a 2nd IO ops while the first is still there
and that's illegal enough that we do a pr_err() for it (though we
don't crash), but let's say we're trying to solve that one.

Looking at it closely, I _think_ the only race in this case is if the
one we're trying to unregister had a deinit() function and we going to
replace it?  If it didn't have a deinit function:

cpu1 (unregister)                 cpu2 (register):
-----------------                 ----------------------
kgdb_unregister_callbacks()
                                  spin_lock() <got>
spin_lock() <blocked>
                                  if (old_dbg_io_ops) <true>
                                    if (has dinit) <false>
                                      print error
                                      spin_unlock()
                                      return -EBUSY
<finish unregister>

The above is fine and is the same thing that would happen if the
whole register function ran before the unregister even started, right?

Also: if the unregister won the race that should also be fine.

So really the problem is this:

cpu1 (unregister)                 cpu2 (register):
-----------------                 ----------------------
kgdb_unregister_callbacks()
                                  spin_lock() <got>
spin_lock() <blocked>
                                  if (old_dbg_io_ops) <true>
                                    if (has dinit) <true>
                                      print Replacing
                                  init new IO ops
                                  spin_unlock()
                                  if (old_dbg_io_ops) <true>
                                    finish deinit of old
                                    return true
WARN_ON() <hits and shouts!>
dbg_io_ops = NULL
spin_unlock()
if (deinit) <true>
  double-call to deinit of old

So in this case we'll hit a WARN_ON(), incorrectly unregister the new
IO ops, and call deinit twice.

-Doug