[PATCH] Fix int1 recursion with unregistered breakpoints

[PATCH] Fix int1 recursion with unregistered breakpoints

[Jeff Merkey]

Please consider the attached patch.

I have reviewed all the code that touches this patch and have
determined it will function and support all of the software that
depends on this handler properly.  I have compiled and tested this
patch with a test harness that tests the robustness of the linux
breakpoint API and handlers in the following ways:

1.  Setting multiple conditional breakpoints through
arch_install_hw_breakpoint API across four processors to test the rate
at which the interface can handle breakpoint exceptions

2.  Setting unregistered breakpoints to test the handlers robustness
in dealing with error handling conditions and errant or spurious
hardware conditions and to simulate actual "lazy debug register
switching" (which does not work BTW) with null bp handlers to test the
robustness of the handlers.

3.  Clearing and setting breakpoints across multiple processors then
triggering concurrent exceptions in both interrupt and process
contexts.

This patch improves robustness in several ways in the linux kernel:

1.  Corrects bug in handling unregistered breakpoints.

2.  Provides hardware check of dr7 to determine source of breakpoint
if OS cannot ascertain the int1 source from its own state and
variables.

3.  Actually allows "lazy debug register switching" to function, which
until recently has apparently never been actually seen on live
hardware or actually tested.

Signed-off-by: Jeff Merkey <linux.mdb@gmail.com>
---
 arch/x86/kernel/hw_breakpoint.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 50a3fad..ca13db0 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;
 
@@ -475,6 +475,13 @@ static int hw_breakpoint_handler(struct die_args *args)
 	for (i = 0; i < HBP_NUM; ++i) {
 		if (likely(!(dr6 & (DR_TRAP0 << i))))
 			continue;
+		/*
+		* check if we got an execute breakpoint
+		* from the dr7 register.  if we did, set
+		* the resume flag to avoid int1 recursion.
+		*/
+		if ((dr7 & (3 << ((i * 4) + 16))) == 0)
+			args->regs->flags |= X86_EFLAGS_RF;
 
 		/*
 		 * The counter may be concurrently released but that can only
@@ -503,7 +510,9 @@ static int hw_breakpoint_handler(struct die_args *args)
 
 		/*
 		 * Set up resume flag to avoid breakpoint recursion when
-		 * returning back to origin.
+		 * returning back to origin.  Perform the check
+		* twice in case the event handler altered the
+		* system flags.
 		 */
 		if (bp->hw.info.type == X86_BREAKPOINT_EXECUTE)
 			args->regs->flags |= X86_EFLAGS_RF;
@@ -519,6 +528,18 @@ static int hw_breakpoint_handler(struct die_args *args)
 	    (dr6 & (~DR_TRAP_BITS)))
 		rc = NOTIFY_DONE;
 
+	/*
+	* if we are about to signal to
+	* do_debug() to stop further processing
+	* and we have not ascertained the source
+	* of the breakpoint, log it as spurious.
+	*/
+	if (rc == NOTIFY_STOP && !bp) {
+		printk_ratelimited(KERN_INFO
+				"INFO: spurious INT1 exception dr6: 0x%lX dr7: 0x%lX\n",
+				dr6, dr7);
+	}
+
 	set_debugreg(dr7, 7);
 	put_cpu();
 
-- 
1.8.3.1

[PATCH] Fix int1 recursion with unregistered breakpoints

[Jeff Merkey]

Please consider the attached patch.

I have reviewed all the code that touches this patch and have
determined it will function and support all of the software that
depends on this handler properly.  I have compiled and tested this
patch with a test harness that tests the robustness of the linux
breakpoint API and handlers in the following ways:

1.  Setting multiple conditional breakpoints through
arch_install_hw_breakpoint API across four processors to test the rate
at which the interface can handle breakpoint exceptions

2.  Setting unregistered breakpoints to test the handlers robustness
in dealing with error handling conditions and errant or spurious
hardware conditions and to simulate actual "lazy debug register
switching" (which does not work BTW) with null bp handlers to test the
robustness of the handlers.

3.  Clearing and setting breakpoints across multiple processors then
triggering concurrent exceptions in both interrupt and process
contexts.

This patch improves robustness in several ways in the linux kernel:

1.  Corrects bug in handling unregistered breakpoints.

2.  Provides hardware check of dr7 to determine source of breakpoint
if OS cannot ascertain the int1 source from its own state and
variables.

3.  Actually allows "lazy debug register switching" to function, which
until recently has apparently never been actually seen on live
hardware or actually tested.

Signed-off-by: Jeff Merkey <linux.mdb@gmail.com>
---
 arch/x86/kernel/hw_breakpoint.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 50a3fad..ca13db0 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;
 
@@ -475,6 +475,13 @@ static int hw_breakpoint_handler(struct die_args *args)
 	for (i = 0; i < HBP_NUM; ++i) {
 		if (likely(!(dr6 & (DR_TRAP0 << i))))
 			continue;
+		/*
+		* check if we got an execute breakpoint
+		* from the dr7 register.  if we did, set
+		* the resume flag to avoid int1 recursion.
+		*/
+		if ((dr7 & (3 << ((i * 4) + 16))) == 0)
+			args->regs->flags |= X86_EFLAGS_RF;
 
 		/*
 		 * The counter may be concurrently released but that can only
@@ -503,7 +510,9 @@ static int hw_breakpoint_handler(struct die_args *args)
 
 		/*
 		 * Set up resume flag to avoid breakpoint recursion when
-		 * returning back to origin.
+		 * returning back to origin.  Perform the check
+		* twice in case the event handler altered the
+		* system flags.
 		 */
 		if (bp->hw.info.type == X86_BREAKPOINT_EXECUTE)
 			args->regs->flags |= X86_EFLAGS_RF;
@@ -519,6 +528,18 @@ static int hw_breakpoint_handler(struct die_args *args)
 	    (dr6 & (~DR_TRAP_BITS)))
 		rc = NOTIFY_DONE;
 
+	/*
+	* if we are about to signal to
+	* do_debug() to stop further processing
+	* and we have not ascertained the source
+	* of the breakpoint, log it as spurious.
+	*/
+	if (rc == NOTIFY_STOP && !bp) {
+		printk_ratelimited(KERN_INFO
+				"INFO: spurious INT1 exception dr6: 0x%lX dr7: 0x%lX\n",
+				dr6, dr7);
+	}
+
 	set_debugreg(dr7, 7);
 	put_cpu();
 
-- 
1.8.3.1

Re: [PATCH] Fix int1 recursion with unregistered breakpoints

[H. Peter Anvin]

On 12/14/15 13:03, Jeff Merkey wrote:
> Please consider the attached patch.
> 
> I have reviewed all the code that touches this patch and have
> determined it will function and support all of the software that
> depends on this handler properly.  I have compiled and tested this
> patch with a test harness that tests the robustness of the linux
> breakpoint API and handlers in the following ways:
> 
> 1.  Setting multiple conditional breakpoints through
> arch_install_hw_breakpoint API across four processors to test the rate
> at which the interface can handle breakpoint exceptions
> 
> 2.  Setting unregistered breakpoints to test the handlers robustness
> in dealing with error handling conditions and errant or spurious
> hardware conditions and to simulate actual "lazy debug register
> switching" (which does not work BTW) with null bp handlers to test the
> robustness of the handlers.
> 
> 3.  Clearing and setting breakpoints across multiple processors then
> triggering concurrent exceptions in both interrupt and process
> contexts.
> 
> This patch improves robustness in several ways in the linux kernel:
> 
> 1.  Corrects bug in handling unregistered breakpoints.
> 
> 2.  Provides hardware check of dr7 to determine source of breakpoint
> if OS cannot ascertain the int1 source from its own state and
> variables.
> 
> 3.  Actually allows "lazy debug register switching" to function, which
> until recently has apparently never been actually seen on live
> hardware or actually tested.
> 

This is all fine and good, but you are missing one of the most important
parts of a patch: a patch description, describing in detail the problem
that it solves and why.  This description needs to be comprehensible not
just for people already initiated but for someone doing code archaeology
a decade from now.

Thanks,

	-hpa

Re: [PATCH] Fix int1 recursion with unregistered breakpoints

[Jeff Merkey]

On 12/14/15, H. Peter Anvin <hpa@zytor.com> wrote:
> On 12/14/15 13:03, Jeff Merkey wrote:
>> Please consider the attached patch.
>>
>> I have reviewed all the code that touches this patch and have
>> determined it will function and support all of the software that
>> depends on this handler properly.  I have compiled and tested this
>> patch with a test harness that tests the robustness of the linux
>> breakpoint API and handlers in the following ways:
>>
>> 1.  Setting multiple conditional breakpoints through
>> arch_install_hw_breakpoint API across four processors to test the rate
>> at which the interface can handle breakpoint exceptions
>>
>> 2.  Setting unregistered breakpoints to test the handlers robustness
>> in dealing with error handling conditions and errant or spurious
>> hardware conditions and to simulate actual "lazy debug register
>> switching" (which does not work BTW) with null bp handlers to test the
>> robustness of the handlers.
>>
>> 3.  Clearing and setting breakpoints across multiple processors then
>> triggering concurrent exceptions in both interrupt and process
>> contexts.
>>
>> This patch improves robustness in several ways in the linux kernel:
>>
>> 1.  Corrects bug in handling unregistered breakpoints.
>>
>> 2.  Provides hardware check of dr7 to determine source of breakpoint
>> if OS cannot ascertain the int1 source from its own state and
>> variables.
>>
>> 3.  Actually allows "lazy debug register switching" to function, which
>> until recently has apparently never been actually seen on live
>> hardware or actually tested.
>>
>
> This is all fine and good, but you are missing one of the most important
> parts of a patch: a patch description, describing in detail the problem
> that it solves and why.  This description needs to be comprehensible not
> just for people already initiated but for someone doing code archaeology
> a decade from now.
>
> Thanks,
>
> 	-hpa
>
>
>

Yes sir, I'll get that added right away.

Jeff