[v2,2/3] kgdb: Use the kprobe blocklist to limit single stepping

Message ID 20200716151943.2167652-3-daniel.thompson@linaro.org
State New
Headers show
Series
  • kgdb: Honour the kprobe blacklist when setting breakpoints
Related show

Commit Message

Daniel Thompson July 16, 2020, 3:19 p.m.
If we are running in a part of the kernel that dislikes breakpoint
debugging then it is very unlikely to be safe to single step. Add
some safety rails to prevent stepping through anything on the kprobe
blocklist.

As part of this kdb_ss() will no longer set the DOING_SS flags when it
requests a step. This is safe because this flag is already redundant,
returning KDB_CMD_SS is all that is needed to request a step (and this
saves us from having to unset the flag if the safety check fails).

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

---
 include/linux/kgdb.h        |  1 +
 kernel/debug/debug_core.c   | 13 +++++++++++++
 kernel/debug/gdbstub.c      | 10 +++++++++-
 kernel/debug/kdb/kdb_bp.c   |  8 ++------
 kernel/debug/kdb/kdb_main.c | 10 ++++++++--
 5 files changed, 33 insertions(+), 9 deletions(-)

-- 
2.25.4

Comments

Doug Anderson July 17, 2020, 10:39 p.m. | #1
Hi,

On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>

> If we are running in a part of the kernel that dislikes breakpoint

> debugging then it is very unlikely to be safe to single step. Add

> some safety rails to prevent stepping through anything on the kprobe

> blocklist.

>

> As part of this kdb_ss() will no longer set the DOING_SS flags when it

> requests a step. This is safe because this flag is already redundant,

> returning KDB_CMD_SS is all that is needed to request a step (and this

> saves us from having to unset the flag if the safety check fails).

>

> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> ---

>  include/linux/kgdb.h        |  1 +

>  kernel/debug/debug_core.c   | 13 +++++++++++++

>  kernel/debug/gdbstub.c      | 10 +++++++++-

>  kernel/debug/kdb/kdb_bp.c   |  8 ++------

>  kernel/debug/kdb/kdb_main.c | 10 ++++++++--

>  5 files changed, 33 insertions(+), 9 deletions(-)

>

> diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h

> index 7caba4604edc..aefe823998cb 100644

> --- a/include/linux/kgdb.h

> +++ b/include/linux/kgdb.h

> @@ -214,6 +214,7 @@ extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);

>

>  /* Optional functions. */

>  extern int kgdb_validate_break_address(unsigned long addr);

> +extern int kgdb_validate_single_step_address(unsigned long addr);

>  extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);

>  extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);

>

> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c

> index 133a361578dc..4b59bcc90c5d 100644

> --- a/kernel/debug/debug_core.c

> +++ b/kernel/debug/debug_core.c

> @@ -208,6 +208,19 @@ int __weak kgdb_validate_break_address(unsigned long addr)

>         return err;

>  }

>

> +int __weak kgdb_validate_single_step_address(unsigned long addr)

> +{

> +       /*

> +        * Disallow stepping when we are executing code that is marked

> +        * as unsuitable for breakpointing... stepping won't be safe

> +        * either!

> +        */

> +       if (kgdb_within_blocklist(addr))

> +               return -EINVAL;

> +

> +       return 0;

> +}

> +

>  unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)

>  {

>         return instruction_pointer(regs);

> diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c

> index 61774aec46b4..f1c88007cc2b 100644

> --- a/kernel/debug/gdbstub.c

> +++ b/kernel/debug/gdbstub.c

> @@ -1041,8 +1041,16 @@ int gdb_serial_stub(struct kgdb_state *ks)

>                         if (tmp == 0)

>                                 break;

>                         /* Fall through - on tmp < 0 */

> -               case 'c': /* Continue packet */

>                 case 's': /* Single step packet */

> +                       error = kgdb_validate_single_step_address(

> +                                       kgdb_arch_pc(ks->ex_vector,

> +                                                    ks->linux_regs));


I'm a little confused.  Isn't this like saying "if
(i_am_standing_in_acid) dont_step_into_acid"?

Specifically you're checking the _current_ PC to see if it's in the
blocklist, right?  ...but you've already (effectively) dropped into
the debugger at that location, so if it really was a problem wouldn't
we already be in trouble?

What you really want (I think?) is to know if the instruction that
you're stepping into is in the blocklist, right?  ...but you can't
know that because it requires a full instruction emulator (that's why
CPUs have "single step mode").

I guess you get a marginal benefit if someone manually set their
instruction pointer to be an address in the middle of a blocklisted
function and then trying to step, but I'm not sure that's really
something we need to add code for?

It feels like the right solution is that the architecture-specific
single-step code should simply consider a single-step through a
blocklisted area to be a step through one giant instruction.


> +                       if (error != 0) {

> +                               error_packet(remcom_out_buffer, error);

> +                               break;

> +                       }

> +                       fallthrough;

> +               case 'c': /* Continue packet */

>                         if (kgdb_contthread && kgdb_contthread != current) {

>                                 /* Can't switch threads in kgdb */

>                                 error_packet(remcom_out_buffer, -EINVAL);

> diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c

> index ec4940146612..4853c413f579 100644

> --- a/kernel/debug/kdb/kdb_bp.c

> +++ b/kernel/debug/kdb/kdb_bp.c

> @@ -507,18 +507,14 @@ static int kdb_bc(int argc, const char **argv)

>   *     None.

>   * Remarks:

>   *

> - *     Set the arch specific option to trigger a debug trap after the next

> - *     instruction.

> + *     KDB_CMD_SS is a command that our caller acts on to effect the step.

>   */

>

>  static int kdb_ss(int argc, const char **argv)

>  {

>         if (argc != 0)

>                 return KDB_ARGCOUNT;

> -       /*

> -        * Set trace flag and go.

> -        */

> -       KDB_STATE_SET(DOING_SS);

> +

>         return KDB_CMD_SS;

>  }

>

> diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c

> index 5c7949061671..cd40bf780b93 100644

> --- a/kernel/debug/kdb/kdb_main.c

> +++ b/kernel/debug/kdb/kdb_main.c

> @@ -1189,7 +1189,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,

>                      kdb_dbtrap_t db_result)

>  {

>         char *cmdbuf;

> -       int diag;

> +       int diag, res;

>         struct task_struct *kdb_current =

>                 kdb_curr_task(raw_smp_processor_id());

>

> @@ -1346,10 +1346,16 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,

>                 }

>                 if (diag == KDB_CMD_GO

>                  || diag == KDB_CMD_CPU

> -                || diag == KDB_CMD_SS

>                  || diag == KDB_CMD_KGDB)

>                         break;

>

> +               if (diag == KDB_CMD_SS) {

> +                       res = kgdb_validate_single_step_address(instruction_pointer(regs));


Is it legit to use instruction_pointer() directly?  Should you be
calling kgdb_arch_pc()  ...or does that just account for having just
hit a breakpoint?
Daniel Thompson July 20, 2020, 8:07 a.m. | #2
On Fri, Jul 17, 2020 at 03:39:51PM -0700, Doug Anderson wrote:
> Hi,

> 

> On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson

> <daniel.thompson@linaro.org> wrote:

> >

> > If we are running in a part of the kernel that dislikes breakpoint

> > debugging then it is very unlikely to be safe to single step. Add

> > some safety rails to prevent stepping through anything on the kprobe

> > blocklist.

> >

> > As part of this kdb_ss() will no longer set the DOING_SS flags when it

> > requests a step. This is safe because this flag is already redundant,

> > returning KDB_CMD_SS is all that is needed to request a step (and this

> > saves us from having to unset the flag if the safety check fails).

> >

> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > ---

> >  include/linux/kgdb.h        |  1 +

> >  kernel/debug/debug_core.c   | 13 +++++++++++++

> >  kernel/debug/gdbstub.c      | 10 +++++++++-

> >  kernel/debug/kdb/kdb_bp.c   |  8 ++------

> >  kernel/debug/kdb/kdb_main.c | 10 ++++++++--

> >  5 files changed, 33 insertions(+), 9 deletions(-)

> >

> > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h

> > index 7caba4604edc..aefe823998cb 100644

> > --- a/include/linux/kgdb.h

> > +++ b/include/linux/kgdb.h

> > @@ -214,6 +214,7 @@ extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);

> >

> >  /* Optional functions. */

> >  extern int kgdb_validate_break_address(unsigned long addr);

> > +extern int kgdb_validate_single_step_address(unsigned long addr);

> >  extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);

> >  extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);

> >

> > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c

> > index 133a361578dc..4b59bcc90c5d 100644

> > --- a/kernel/debug/debug_core.c

> > +++ b/kernel/debug/debug_core.c

> > @@ -208,6 +208,19 @@ int __weak kgdb_validate_break_address(unsigned long addr)

> >         return err;

> >  }

> >

> > +int __weak kgdb_validate_single_step_address(unsigned long addr)

> > +{

> > +       /*

> > +        * Disallow stepping when we are executing code that is marked

> > +        * as unsuitable for breakpointing... stepping won't be safe

> > +        * either!

> > +        */

> > +       if (kgdb_within_blocklist(addr))

> > +               return -EINVAL;

> > +

> > +       return 0;

> > +}

> > +

> >  unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)

> >  {

> >         return instruction_pointer(regs);

> > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c

> > index 61774aec46b4..f1c88007cc2b 100644

> > --- a/kernel/debug/gdbstub.c

> > +++ b/kernel/debug/gdbstub.c

> > @@ -1041,8 +1041,16 @@ int gdb_serial_stub(struct kgdb_state *ks)

> >                         if (tmp == 0)

> >                                 break;

> >                         /* Fall through - on tmp < 0 */

> > -               case 'c': /* Continue packet */

> >                 case 's': /* Single step packet */

> > +                       error = kgdb_validate_single_step_address(

> > +                                       kgdb_arch_pc(ks->ex_vector,

> > +                                                    ks->linux_regs));

> 

> I'm a little confused.  Isn't this like saying "if

> (i_am_standing_in_acid) dont_step_into_acid"?


I describe it more as:

    if (we_know_there_is_acid_nearby)
        dont_step_forward

It is possible we are currently stepping in acid but it is also possible
(and reasonably likely) that we haven't stepped in it yet but will do so
soon.


> Specifically you're checking the _current_ PC to see if it's in the

> blocklist, right?  ...but you've already (effectively) dropped into

> the debugger at that location, so if it really was a problem wouldn't

> we already be in trouble?


The basic use case is where someone is stepping and we reach a PC that
would be blocked for a breakpoint. This will typically be due (although
I think it does generalize) to a function call and the safety rail will
be reached after we have jumped to the blocked function but before we
actually execute any instructions within it.

Or putting it another way, there is no reason to worry if we start
somewhere "safe" and start stepping towards something on the blocklist.
We won't melt our shoes!

There are more complex cases when we drop into the debugger in the
middle of blocked code with a not-breakpoint-or-step trap. You're right
that we'd been in touble and the debugger it probably a bit fragile.
However that certainly doesn't mean blocking stepping at this point
is a bad thing!


> What you really want (I think?) is to know if the instruction that

> you're stepping into is in the blocklist, right?  ...but you can't

> know that because it requires a full instruction emulator (that's why

> CPUs have "single step mode").


As above, I don't think this is needed but if there was an architecture
that did then it can override the default implementation if it wanted
to.

 
> I guess you get a marginal benefit if someone manually set their

> instruction pointer to be an address in the middle of a blocklisted

> function and then trying to step, but I'm not sure that's really

> something we need to add code for?


Perhaps off-topic given this isn't why we add the satefy rails but...

I think people who directly set PC should be regarded as very
sophisticated users (and therefore do not need safety rails) so I have
little interest in honouring the blocklist for direct writes to the
PC. More generally sophisticated users should be able to find
KGDB_HONOUR_BLOCKLIST pretty quickly if they need to!


> It feels like the right solution is that the architecture-specific

> single-step code should simply consider a single-step through a

> blocklisted area to be a step through one giant instruction.


For kgdb this feature is already implemented (next or finish).


> > +                       if (error != 0) {

> > +                               error_packet(remcom_out_buffer, error);

> > +                               break;

> > +                       }

> > +                       fallthrough;

> > +               case 'c': /* Continue packet */

> >                         if (kgdb_contthread && kgdb_contthread != current) {

> >                                 /* Can't switch threads in kgdb */

> >                                 error_packet(remcom_out_buffer, -EINVAL);

> > diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c

> > index ec4940146612..4853c413f579 100644

> > --- a/kernel/debug/kdb/kdb_bp.c

> > +++ b/kernel/debug/kdb/kdb_bp.c

> > @@ -507,18 +507,14 @@ static int kdb_bc(int argc, const char **argv)

> >   *     None.

> >   * Remarks:

> >   *

> > - *     Set the arch specific option to trigger a debug trap after the next

> > - *     instruction.

> > + *     KDB_CMD_SS is a command that our caller acts on to effect the step.

> >   */

> >

> >  static int kdb_ss(int argc, const char **argv)

> >  {

> >         if (argc != 0)

> >                 return KDB_ARGCOUNT;

> > -       /*

> > -        * Set trace flag and go.

> > -        */

> > -       KDB_STATE_SET(DOING_SS);

> > +

> >         return KDB_CMD_SS;

> >  }

> >

> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c

> > index 5c7949061671..cd40bf780b93 100644

> > --- a/kernel/debug/kdb/kdb_main.c

> > +++ b/kernel/debug/kdb/kdb_main.c

> > @@ -1189,7 +1189,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,

> >                      kdb_dbtrap_t db_result)

> >  {

> >         char *cmdbuf;

> > -       int diag;

> > +       int diag, res;

> >         struct task_struct *kdb_current =

> >                 kdb_curr_task(raw_smp_processor_id());

> >

> > @@ -1346,10 +1346,16 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,

> >                 }

> >                 if (diag == KDB_CMD_GO

> >                  || diag == KDB_CMD_CPU

> > -                || diag == KDB_CMD_SS

> >                  || diag == KDB_CMD_KGDB)

> >                         break;

> >

> > +               if (diag == KDB_CMD_SS) {

> > +                       res = kgdb_validate_single_step_address(instruction_pointer(regs));

> 

> Is it legit to use instruction_pointer() directly?  Should you be

> calling kgdb_arch_pc()  ...or does that just account for having just

> hit a breakpoint?


I decided between kgdb_arch_pc() and instruction_pointer() based on the
usage of regs in the rest of this file (which is exclusively
instruction_pointer() ). I didn't want the lookup to mismatch what the
user has been told in the console.

On the other hand, I did cross my mind that every PC lookup could be
broken and I made a note for the future...


Daniel.
Doug Anderson July 21, 2020, 9:04 p.m. | #3
Hi,

On Mon, Jul 20, 2020 at 1:08 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>

> On Fri, Jul 17, 2020 at 03:39:51PM -0700, Doug Anderson wrote:

> > Hi,

> >

> > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson

> > <daniel.thompson@linaro.org> wrote:

> > >

> > > If we are running in a part of the kernel that dislikes breakpoint

> > > debugging then it is very unlikely to be safe to single step. Add

> > > some safety rails to prevent stepping through anything on the kprobe

> > > blocklist.

> > >

> > > As part of this kdb_ss() will no longer set the DOING_SS flags when it

> > > requests a step. This is safe because this flag is already redundant,

> > > returning KDB_CMD_SS is all that is needed to request a step (and this

> > > saves us from having to unset the flag if the safety check fails).

> > >

> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > > ---

> > >  include/linux/kgdb.h        |  1 +

> > >  kernel/debug/debug_core.c   | 13 +++++++++++++

> > >  kernel/debug/gdbstub.c      | 10 +++++++++-

> > >  kernel/debug/kdb/kdb_bp.c   |  8 ++------

> > >  kernel/debug/kdb/kdb_main.c | 10 ++++++++--

> > >  5 files changed, 33 insertions(+), 9 deletions(-)

> > >

> > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h

> > > index 7caba4604edc..aefe823998cb 100644

> > > --- a/include/linux/kgdb.h

> > > +++ b/include/linux/kgdb.h

> > > @@ -214,6 +214,7 @@ extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);

> > >

> > >  /* Optional functions. */

> > >  extern int kgdb_validate_break_address(unsigned long addr);

> > > +extern int kgdb_validate_single_step_address(unsigned long addr);

> > >  extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);

> > >  extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);

> > >

> > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c

> > > index 133a361578dc..4b59bcc90c5d 100644

> > > --- a/kernel/debug/debug_core.c

> > > +++ b/kernel/debug/debug_core.c

> > > @@ -208,6 +208,19 @@ int __weak kgdb_validate_break_address(unsigned long addr)

> > >         return err;

> > >  }

> > >

> > > +int __weak kgdb_validate_single_step_address(unsigned long addr)

> > > +{

> > > +       /*

> > > +        * Disallow stepping when we are executing code that is marked

> > > +        * as unsuitable for breakpointing... stepping won't be safe

> > > +        * either!

> > > +        */

> > > +       if (kgdb_within_blocklist(addr))

> > > +               return -EINVAL;

> > > +

> > > +       return 0;

> > > +}

> > > +

> > >  unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)

> > >  {

> > >         return instruction_pointer(regs);

> > > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c

> > > index 61774aec46b4..f1c88007cc2b 100644

> > > --- a/kernel/debug/gdbstub.c

> > > +++ b/kernel/debug/gdbstub.c

> > > @@ -1041,8 +1041,16 @@ int gdb_serial_stub(struct kgdb_state *ks)

> > >                         if (tmp == 0)

> > >                                 break;

> > >                         /* Fall through - on tmp < 0 */

> > > -               case 'c': /* Continue packet */

> > >                 case 's': /* Single step packet */

> > > +                       error = kgdb_validate_single_step_address(

> > > +                                       kgdb_arch_pc(ks->ex_vector,

> > > +                                                    ks->linux_regs));

> >

> > I'm a little confused.  Isn't this like saying "if

> > (i_am_standing_in_acid) dont_step_into_acid"?

>

> I describe it more as:

>

>     if (we_know_there_is_acid_nearby)

>         dont_step_forward

>

> It is possible we are currently stepping in acid but it is also possible

> (and reasonably likely) that we haven't stepped in it yet but will do so

> soon.

>

>

> > Specifically you're checking the _current_ PC to see if it's in the

> > blocklist, right?  ...but you've already (effectively) dropped into

> > the debugger at that location, so if it really was a problem wouldn't

> > we already be in trouble?

>

> The basic use case is where someone is stepping and we reach a PC that

> would be blocked for a breakpoint. This will typically be due (although

> I think it does generalize) to a function call and the safety rail will

> be reached after we have jumped to the blocked function but before we

> actually execute any instructions within it.

>

> Or putting it another way, there is no reason to worry if we start

> somewhere "safe" and start stepping towards something on the blocklist.

> We won't melt our shoes!


I guess I still don't totally get it.  So let's say we have:

void dont_trace_this(...)
{
  thing_not_to_trace_1();
  thing_not_to_trace_2();
  don_t_trace = this;
}
NOKPROBE_SYMBOL(dont_trace_this);

void trace_me()
{
  sing();
  dance();
  dont_trace_this();
  party();
}

So presumably the dont_trace_this() function is marked as
NOKPROBE_SYMBOL because it's called by the kprobe handling code or by
kgdb, right?  So if we had a breakpoint there then we'd just have
infinite recursion.  Thus we want to prevent putting breakpoints
anywhere in this function.  Even though dont_trace_this() is also
called from the trace_me() function it doesn't matter--we still can't
put breakpoints in it because it would cause problems with the
debugger.

Now, I guess the question is: why exactly do we need to prevent single
stepping in dont_trace_this().  In the case above where
dont_trace_this() is called from trace_me() it would actually be OK to
single step it, right?  ...unless this is on a CPU that doesn't have a
"single step mode" and has to implement stepping by breakpoints, of
course.

...but maybe I'm confused and there is a reason that we shouldn't
allow single stepping into dont_trace_this() when called from
trace_me().  If that is the case, I'm wondering why it's OK to step
and stop on the first instruction of the function but it's not OK to
step and stop through the other instructions in the function.

-Doug
Daniel Thompson Sept. 4, 2020, 3:22 p.m. | #4
On Tue, Jul 21, 2020 at 02:04:45PM -0700, Doug Anderson wrote:
> On Mon, Jul 20, 2020 at 1:08 AM Daniel Thompson

> <daniel.thompson@linaro.org> wrote:

> > On Fri, Jul 17, 2020 at 03:39:51PM -0700, Doug Anderson wrote:

> > > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson

> > > <daniel.thompson@linaro.org> wrote:

> > > >

> > > > If we are running in a part of the kernel that dislikes breakpoint

> > > > debugging then it is very unlikely to be safe to single step. Add

> > > > some safety rails to prevent stepping through anything on the kprobe

> > > > blocklist.

> > > >

> > > > As part of this kdb_ss() will no longer set the DOING_SS flags when it

> > > > requests a step. This is safe because this flag is already redundant,

> > > > returning KDB_CMD_SS is all that is needed to request a step (and this

> > > > saves us from having to unset the flag if the safety check fails).

> > > >

> > > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > > > ---

> > > >  include/linux/kgdb.h        |  1 +

> > > >  kernel/debug/debug_core.c   | 13 +++++++++++++

> > > >  kernel/debug/gdbstub.c      | 10 +++++++++-

> > > >  kernel/debug/kdb/kdb_bp.c   |  8 ++------

> > > >  kernel/debug/kdb/kdb_main.c | 10 ++++++++--

> > > >  5 files changed, 33 insertions(+), 9 deletions(-)

> > > >

> > > > diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h

> > > > index 7caba4604edc..aefe823998cb 100644

> > > > --- a/include/linux/kgdb.h

> > > > +++ b/include/linux/kgdb.h

> > > > @@ -214,6 +214,7 @@ extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);

> > > >

> > > >  /* Optional functions. */

> > > >  extern int kgdb_validate_break_address(unsigned long addr);

> > > > +extern int kgdb_validate_single_step_address(unsigned long addr);

> > > >  extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);

> > > >  extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);

> > > >

> > > > diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c

> > > > index 133a361578dc..4b59bcc90c5d 100644

> > > > --- a/kernel/debug/debug_core.c

> > > > +++ b/kernel/debug/debug_core.c

> > > > @@ -208,6 +208,19 @@ int __weak kgdb_validate_break_address(unsigned long addr)

> > > >         return err;

> > > >  }

> > > >

> > > > +int __weak kgdb_validate_single_step_address(unsigned long addr)

> > > > +{

> > > > +       /*

> > > > +        * Disallow stepping when we are executing code that is marked

> > > > +        * as unsuitable for breakpointing... stepping won't be safe

> > > > +        * either!

> > > > +        */

> > > > +       if (kgdb_within_blocklist(addr))

> > > > +               return -EINVAL;

> > > > +

> > > > +       return 0;

> > > > +}

> > > > +

> > > >  unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)

> > > >  {

> > > >         return instruction_pointer(regs);

> > > > diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c

> > > > index 61774aec46b4..f1c88007cc2b 100644

> > > > --- a/kernel/debug/gdbstub.c

> > > > +++ b/kernel/debug/gdbstub.c

> > > > @@ -1041,8 +1041,16 @@ int gdb_serial_stub(struct kgdb_state *ks)

> > > >                         if (tmp == 0)

> > > >                                 break;

> > > >                         /* Fall through - on tmp < 0 */

> > > > -               case 'c': /* Continue packet */

> > > >                 case 's': /* Single step packet */

> > > > +                       error = kgdb_validate_single_step_address(

> > > > +                                       kgdb_arch_pc(ks->ex_vector,

> > > > +                                                    ks->linux_regs));

> > >

> > > I'm a little confused.  Isn't this like saying "if

> > > (i_am_standing_in_acid) dont_step_into_acid"?

> >

> > I describe it more as:

> >

> >     if (we_know_there_is_acid_nearby)

> >         dont_step_forward

> >

> > It is possible we are currently stepping in acid but it is also possible

> > (and reasonably likely) that we haven't stepped in it yet but will do so

> > soon.

> >

> >

> > > Specifically you're checking the _current_ PC to see if it's in the

> > > blocklist, right?  ...but you've already (effectively) dropped into

> > > the debugger at that location, so if it really was a problem wouldn't

> > > we already be in trouble?

> >

> > The basic use case is where someone is stepping and we reach a PC that

> > would be blocked for a breakpoint. This will typically be due (although

> > I think it does generalize) to a function call and the safety rail will

> > be reached after we have jumped to the blocked function but before we

> > actually execute any instructions within it.

> >

> > Or putting it another way, there is no reason to worry if we start

> > somewhere "safe" and start stepping towards something on the blocklist.

> > We won't melt our shoes!

> 

> I guess I still don't totally get it.  So let's say we have:

> 

> void dont_trace_this(...)

> {

>   thing_not_to_trace_1();

>   thing_not_to_trace_2();

>   don_t_trace = this;

> }

> NOKPROBE_SYMBOL(dont_trace_this);

> 

> void trace_me()

> {

>   sing();

>   dance();

>   dont_trace_this();

>   party();

> }

> 

> So presumably the dont_trace_this() function is marked as

> NOKPROBE_SYMBOL because it's called by the kprobe handling code or by

> kgdb, right?  So if we had a breakpoint there then we'd just have

> infinite recursion.  Thus we want to prevent putting breakpoints

> anywhere in this function.  Even though dont_trace_this() is also

> called from the trace_me() function it doesn't matter--we still can't

> put breakpoints in it because it would cause problems with the

> debugger.

> 

> Now, I guess the question is: why exactly do we need to prevent single

> stepping in dont_trace_this().  In the case above where

> dont_trace_this() is called from trace_me() it would actually be OK to

> single step it, right?  ...unless this is on a CPU that doesn't have a

> "single step mode" and has to implement stepping by breakpoints, of

> course.


I think you are persuading me.

Although I can think of plenty of places where it isn't safe to step I'm
struggling to think of any way for us to end up stopped in the debugger
in those places and certainly now without setting the catastrophic
(which is the only safety rail currently extant).

That means I don't think I can put up a strong enough case that this
patch is better than doing nothing!

I'll drop it for now.

> ...but maybe I'm confused


I think on the whole you've expressed things more lucidly than I have!
Nevertheless...

> and there is a reason that we shouldn't

> allow single stepping into dont_trace_this() when called from

> trace_me().  If that is the case, I'm wondering why it's OK to step

> and stop on the first instruction of the function but it's not OK to

> step and stop through the other instructions in the function.


... when we stop on the first instruction of a function then we have not
actually executed any part of it. In other words we haven't executed
anything on the blocklist.

Of course the whole issue is moot for now.


Daniel.

Patch

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 7caba4604edc..aefe823998cb 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -214,6 +214,7 @@  extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);
 
 /* Optional functions. */
 extern int kgdb_validate_break_address(unsigned long addr);
+extern int kgdb_validate_single_step_address(unsigned long addr);
 extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);
 extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);
 
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 133a361578dc..4b59bcc90c5d 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -208,6 +208,19 @@  int __weak kgdb_validate_break_address(unsigned long addr)
 	return err;
 }
 
+int __weak kgdb_validate_single_step_address(unsigned long addr)
+{
+	/*
+	 * Disallow stepping when we are executing code that is marked
+	 * as unsuitable for breakpointing... stepping won't be safe
+	 * either!
+	 */
+	if (kgdb_within_blocklist(addr))
+		return -EINVAL;
+
+	return 0;
+}
+
 unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)
 {
 	return instruction_pointer(regs);
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 61774aec46b4..f1c88007cc2b 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1041,8 +1041,16 @@  int gdb_serial_stub(struct kgdb_state *ks)
 			if (tmp == 0)
 				break;
 			/* Fall through - on tmp < 0 */
-		case 'c': /* Continue packet */
 		case 's': /* Single step packet */
+			error = kgdb_validate_single_step_address(
+					kgdb_arch_pc(ks->ex_vector,
+						     ks->linux_regs));
+			if (error != 0) {
+				error_packet(remcom_out_buffer, error);
+				break;
+			}
+			fallthrough;
+		case 'c': /* Continue packet */
 			if (kgdb_contthread && kgdb_contthread != current) {
 				/* Can't switch threads in kgdb */
 				error_packet(remcom_out_buffer, -EINVAL);
diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
index ec4940146612..4853c413f579 100644
--- a/kernel/debug/kdb/kdb_bp.c
+++ b/kernel/debug/kdb/kdb_bp.c
@@ -507,18 +507,14 @@  static int kdb_bc(int argc, const char **argv)
  *	None.
  * Remarks:
  *
- *	Set the arch specific option to trigger a debug trap after the next
- *	instruction.
+ *	KDB_CMD_SS is a command that our caller acts on to effect the step.
  */
 
 static int kdb_ss(int argc, const char **argv)
 {
 	if (argc != 0)
 		return KDB_ARGCOUNT;
-	/*
-	 * Set trace flag and go.
-	 */
-	KDB_STATE_SET(DOING_SS);
+
 	return KDB_CMD_SS;
 }
 
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 5c7949061671..cd40bf780b93 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1189,7 +1189,7 @@  static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
 		     kdb_dbtrap_t db_result)
 {
 	char *cmdbuf;
-	int diag;
+	int diag, res;
 	struct task_struct *kdb_current =
 		kdb_curr_task(raw_smp_processor_id());
 
@@ -1346,10 +1346,16 @@  static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
 		}
 		if (diag == KDB_CMD_GO
 		 || diag == KDB_CMD_CPU
-		 || diag == KDB_CMD_SS
 		 || diag == KDB_CMD_KGDB)
 			break;
 
+		if (diag == KDB_CMD_SS) {
+			res = kgdb_validate_single_step_address(instruction_pointer(regs));
+			if (res == 0)
+				break;
+			diag = res;
+		}
+
 		if (diag)
 			kdb_cmderror(diag);
 	}