linux-kernel - Re: [GIT PULL v4.5] Fix INT1 recursion with unregistered breakpoints

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]

Date:	Mon, 11 Jan 2016 16:40:11 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	"Jeff V. Merkey" <linux.mdb@...il.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, X86 ML <x86@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...nel.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Borislav Petkov <bp@...en8.de>, Jiri Olsa <jolsa@...nel.org>
Subject: Re: [GIT PULL v4.5] Fix INT1 recursion with unregistered breakpoints

On Mon, Jan 11, 2016 at 4:35 PM, Jeff V. Merkey <linux.mdb@...il.com> wrote:
> The following changes since commit ee78027142ab39d4f3c0e1af71ed343e0ff2dafd:
>
>   Merge pull request #12 from torvalds/master (2015-12-19 11:12:20 -0700)
>
> are available in the git repository at:
>
>
>   https://github.com/jeffmerkey/linux.git fixes
>
> for you to fetch changes up to b5f894bf53e7c401cc5a88b8a8b13059a176a538:
>
>   Fix INT1 Recursion with unregistered breakpoints (2015-12-19 20:33:59 -0700)
>
> ----------------------------------------------------------------
> Jeff Merkey (1):
>       Fix INT1 Recursion with unregistered breakpoints
>
>  arch/x86/include/uapi/asm/debugreg.h |  1 +
>  arch/x86/kernel/hw_breakpoint.c      | 25 +++++++++++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
> index 3c0874d..78fc83c 100644
> --- a/arch/x86/include/uapi/asm/debugreg.h
> +++ b/arch/x86/include/uapi/asm/debugreg.h
> @@ -38,6 +38,7 @@
>  #define DR_RW_EXECUTE (0x0)   /* Settings for the access types to trap on */
>  #define DR_RW_WRITE (0x1)
>  #define DR_RW_READ (0x3)
> +#define DR_RW_MASK (0x3) /* mask for breakpoint type field */
>
>  #define DR_LEN_1 (0x0) /* Settings for data length to trap on */
>  #define DR_LEN_2 (0x4)
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 50a3fad..d199834 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -444,7 +444,7 @@ EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
>  static int hw_breakpoint_handler(struct die_args *args)
>  {
>         int i, cpu, rc = NOTIFY_STOP;
> -       struct perf_event *bp;
> +       struct perf_event *bp = NULL;
>         unsigned long dr7, dr6;
>         unsigned long *dr6_p;
>
> @@ -477,6 +477,14 @@ static int hw_breakpoint_handler(struct die_args *args)
>                         continue;
>
>                 /*
> +                * Check if we got an execute breakpoint, if so
> +                * set the resume flag to avoid int1 recursion.
> +                */
> +               if (((dr7 >> ((i * DR_CONTROL_SIZE) + DR_CONTROL_SHIFT))
> +                       & DR_RW_MASK) == DR_RW_EXECUTE)
> +                       args->regs->flags |= X86_EFLAGS_RF;
>

I'm still not at all sure I like this, for two reasons:

1. This duplicates the RF-setting logic with all of the legitimate
users.  I can see this blowing up in the case where a breakpoint API
user wants to clear a breakpoint and *not* set RF in case there's
another (possibly just-registered) breakpoint?

2. Shouldn't this clear the spurious breakpoint rather than leaving it set?

In any event, why not just change whatever you're doing to make this
blow up to use the actual breakpoint API?