[PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Alexander Popov]

Make the stack erasing test more verbose about the errors that it
can detect. BUG() in case of test failure is useful when the test
is running in a loop.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 drivers/misc/lkdtm/stackleak.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/lkdtm/stackleak.c b/drivers/misc/lkdtm/stackleak.c
index d5a084475abc..d198de4d4c7e 100644
--- a/drivers/misc/lkdtm/stackleak.c
+++ b/drivers/misc/lkdtm/stackleak.c
@@ -16,6 +16,7 @@ void lkdtm_STACKLEAK_ERASING(void)
 	unsigned long *sp, left, found, i;
 	const unsigned long check_depth =
 			STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
+	bool test_failed = false;
 
 	/*
 	 * For the details about the alignment of the poison values, see
@@ -34,7 +35,8 @@ void lkdtm_STACKLEAK_ERASING(void)
 		left--;
 	} else {
 		pr_err("FAIL: not enough stack space for the test\n");
-		return;
+		test_failed = true;
+		goto end;
 	}
 
 	pr_info("checking unused part of the thread stack (%lu bytes)...\n",
@@ -52,22 +54,29 @@ void lkdtm_STACKLEAK_ERASING(void)
 	}
 
 	if (found <= check_depth) {
-		pr_err("FAIL: thread stack is not erased (checked %lu bytes)\n",
+		pr_err("FAIL: the erased part is not found (checked %lu bytes)\n",
 						i * sizeof(unsigned long));
-		return;
+		test_failed = true;
+		goto end;
 	}
 
-	pr_info("first %lu bytes are unpoisoned\n",
+	pr_info("the erased part begins after %lu not poisoned bytes\n",
 				(i - found) * sizeof(unsigned long));
 
 	/* The rest of thread stack should be erased */
 	for (; i < left; i++) {
 		if (*(sp - i) != STACKLEAK_POISON) {
-			pr_err("FAIL: thread stack is NOT properly erased\n");
-			return;
+			pr_err("FAIL: bad value number %lu in the erased part: 0x%lx\n",
+								i, *(sp - i));
+			test_failed = true;
 		}
 	}
 
-	pr_info("OK: the rest of the thread stack is properly erased\n");
-	return;
+end:
+	if (test_failed) {
+		pr_err("FAIL: the thread stack is NOT properly erased\n");
+		BUG();
+	} else {
+		pr_info("OK: the rest of the thread stack is properly erased\n");
+	}
 }
-- 
2.23.0

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Alexander Popov]

Hello!

Just a friendly ping.
Could I have the feedback for this patch?

Best regards,
Alexander


On 19.12.2019 17:54, Alexander Popov wrote:
> Make the stack erasing test more verbose about the errors that it
> can detect. BUG() in case of test failure is useful when the test
> is running in a loop.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  drivers/misc/lkdtm/stackleak.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/misc/lkdtm/stackleak.c b/drivers/misc/lkdtm/stackleak.c
> index d5a084475abc..d198de4d4c7e 100644
> --- a/drivers/misc/lkdtm/stackleak.c
> +++ b/drivers/misc/lkdtm/stackleak.c
> @@ -16,6 +16,7 @@ void lkdtm_STACKLEAK_ERASING(void)
>  	unsigned long *sp, left, found, i;
>  	const unsigned long check_depth =
>  			STACKLEAK_SEARCH_DEPTH / sizeof(unsigned long);
> +	bool test_failed = false;
>  
>  	/*
>  	 * For the details about the alignment of the poison values, see
> @@ -34,7 +35,8 @@ void lkdtm_STACKLEAK_ERASING(void)
>  		left--;
>  	} else {
>  		pr_err("FAIL: not enough stack space for the test\n");
> -		return;
> +		test_failed = true;
> +		goto end;
>  	}
>  
>  	pr_info("checking unused part of the thread stack (%lu bytes)...\n",
> @@ -52,22 +54,29 @@ void lkdtm_STACKLEAK_ERASING(void)
>  	}
>  
>  	if (found <= check_depth) {
> -		pr_err("FAIL: thread stack is not erased (checked %lu bytes)\n",
> +		pr_err("FAIL: the erased part is not found (checked %lu bytes)\n",
>  						i * sizeof(unsigned long));
> -		return;
> +		test_failed = true;
> +		goto end;
>  	}
>  
> -	pr_info("first %lu bytes are unpoisoned\n",
> +	pr_info("the erased part begins after %lu not poisoned bytes\n",
>  				(i - found) * sizeof(unsigned long));
>  
>  	/* The rest of thread stack should be erased */
>  	for (; i < left; i++) {
>  		if (*(sp - i) != STACKLEAK_POISON) {
> -			pr_err("FAIL: thread stack is NOT properly erased\n");
> -			return;
> +			pr_err("FAIL: bad value number %lu in the erased part: 0x%lx\n",
> +								i, *(sp - i));
> +			test_failed = true;
>  		}
>  	}
>  
> -	pr_info("OK: the rest of the thread stack is properly erased\n");
> -	return;
> +end:
> +	if (test_failed) {
> +		pr_err("FAIL: the thread stack is NOT properly erased\n");
> +		BUG();
> +	} else {
> +		pr_info("OK: the rest of the thread stack is properly erased\n");
> +	}
>  }
> 

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Kees Cook]

On Thu, Dec 19, 2019 at 05:54:16PM +0300, Alexander Popov wrote:
> Make the stack erasing test more verbose about the errors that it
> can detect. BUG() in case of test failure is useful when the test
> is running in a loop.

Hi! I try to keep the "success" conditions for LKDTM tests to be a
system exception, so doing "BUG" on a failure is actually against the
design. So, really, a test harness needs to know to check dmesg for the
results here. It almost looks like this check shouldn't live in LKDTM,
but since it feels like other LKDTM tests, I'm happy to keep it there
for now.

I'll resend my selftests series that adds a real test harness for all
the LKDTM tests and CC you.

-Kees

-- 
Kees Cook

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Alexander Popov]

Hello Kees!

On 30.12.2019 21:37, Kees Cook wrote:
> On Thu, Dec 19, 2019 at 05:54:16PM +0300, Alexander Popov wrote:
>> Make the stack erasing test more verbose about the errors that it
>> can detect. BUG() in case of test failure is useful when the test
>> is running in a loop.
> 
> Hi! I try to keep the "success" conditions for LKDTM tests to be a
> system exception, so doing "BUG" on a failure is actually against the
> design. So, really, a test harness needs to know to check dmesg for the
> results here. It almost looks like this check shouldn't live in LKDTM,

Hm, I see...

Let me explain why I've decided to use BUG() in case of a failure.

Once upon a time I noticed that the stack erasing test failed on a kernel with
KASAN enabled. It happened only once, and all my numerous efforts to reproduce
it failed. That's why I come with this patch. These changes provide additional
information and allow easy detection of a failure when you run the test in a loop.

Is stackleak test the only exception of this kind in LKDTM?

> but since it feels like other LKDTM tests, I'm happy to keep it there
> for now.

Do you mean that you will apply this patch?

> I'll resend my selftests series that adds a real test harness for all
> the LKDTM tests and CC you.

Ok!

Maybe you also see how to improve the LKDTM infrastructure and remove this
inconsistency. Could you share your ideas?

Best regards,
Alexander

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Kees Cook]

On Tue, Dec 31, 2019 at 01:20:24AM +0300, Alexander Popov wrote:
> Hello Kees!
> 
> On 30.12.2019 21:37, Kees Cook wrote:
> > On Thu, Dec 19, 2019 at 05:54:16PM +0300, Alexander Popov wrote:
> >> Make the stack erasing test more verbose about the errors that it
> >> can detect. BUG() in case of test failure is useful when the test
> >> is running in a loop.
> > 
> > Hi! I try to keep the "success" conditions for LKDTM tests to be a
> > system exception, so doing "BUG" on a failure is actually against the
> > design. So, really, a test harness needs to know to check dmesg for the
> > results here. It almost looks like this check shouldn't live in LKDTM,
> 
> Hm, I see...
> 
> Let me explain why I've decided to use BUG() in case of a failure.
> 
> Once upon a time I noticed that the stack erasing test failed on a kernel with
> KASAN enabled. It happened only once, and all my numerous efforts to reproduce
> it failed. That's why I come with this patch. These changes provide additional
> information and allow easy detection of a failure when you run the test in a loop.
> 
> Is stackleak test the only exception of this kind in LKDTM?

Some of the refcount_t tests don't trigger a WARN(), and there are
related benchmarking tests that don't either.

> > but since it feels like other LKDTM tests, I'm happy to keep it there
> > for now.
> 
> Do you mean that you will apply this patch?

Sorry for my confusing reply! I meant that I don't want to apply the
patch, but I'm find to leave the stackleak check in LKDTM.

However, if you want to split it out into its own test, I think that
should be fine; similar to lib/test_user_copy.c if you want it to stand
alone and have its own semantics, etc.

> > I'll resend my selftests series that adds a real test harness for all
> > the LKDTM tests and CC you.
> 
> Ok!
> 
> Maybe you also see how to improve the LKDTM infrastructure and remove this
> inconsistency. Could you share your ideas?

I don't, unfortunately. The real "difficulty" is that some of the
crashes are architecture-specific (e.g. how MMU traps are reported
across different architectures), so it's not too easy to consolidate
the reporting. As a result, I've taken to trying to do best-effort on
the test running side. I'll send what I've got...

-- 
Kees Cook

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Alexander Popov]

On 31.12.2019 01:46, Kees Cook wrote:
> On Tue, Dec 31, 2019 at 01:20:24AM +0300, Alexander Popov wrote:
>> On 30.12.2019 21:37, Kees Cook wrote:
>>> Hi! I try to keep the "success" conditions for LKDTM tests to be a
>>> system exception, so doing "BUG" on a failure is actually against the
>>> design. So, really, a test harness needs to know to check dmesg for the
>>> results here. It almost looks like this check shouldn't live in LKDTM,
>>> but since it feels like other LKDTM tests, I'm happy to keep it there
>>> for now.
>>
>> Do you mean that you will apply this patch?
> 
> Sorry for my confusing reply! I meant that I don't want to apply the
> patch, but I'm find to leave the stackleak check in LKDTM.

Kees, I think I see a solution.

Would you agree if I use dump_stack() instead of BUG() in case of test failure?
That would provide enough info for debugging and would NOT break your design.

Thanks,
Alexander

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Kees Cook]

On Thu, Jan 02, 2020 at 02:26:39AM +0300, Alexander Popov wrote:
> On 31.12.2019 01:46, Kees Cook wrote:
> > On Tue, Dec 31, 2019 at 01:20:24AM +0300, Alexander Popov wrote:
> >> On 30.12.2019 21:37, Kees Cook wrote:
> >>> Hi! I try to keep the "success" conditions for LKDTM tests to be a
> >>> system exception, so doing "BUG" on a failure is actually against the
> >>> design. So, really, a test harness needs to know to check dmesg for the
> >>> results here. It almost looks like this check shouldn't live in LKDTM,
> >>> but since it feels like other LKDTM tests, I'm happy to keep it there
> >>> for now.
> >>
> >> Do you mean that you will apply this patch?
> > 
> > Sorry for my confusing reply! I meant that I don't want to apply the
> > patch, but I'm find to leave the stackleak check in LKDTM.
> 
> Kees, I think I see a solution.
> 
> Would you agree if I use dump_stack() instead of BUG() in case of test failure?
> That would provide enough info for debugging and would NOT break your design.

I would be fine with that, yes! :)

-- 
Kees Cook

Re: [PATCH v1 1/1] lkdtm/stackleak: Make the stack erasing test more verbose

[Alexander Popov]

On 03.01.2020 01:03, Kees Cook wrote:
> On Thu, Jan 02, 2020 at 02:26:39AM +0300, Alexander Popov wrote:
>> On 31.12.2019 01:46, Kees Cook wrote:
>>> On Tue, Dec 31, 2019 at 01:20:24AM +0300, Alexander Popov wrote:
>>>> On 30.12.2019 21:37, Kees Cook wrote:
>>>>> Hi! I try to keep the "success" conditions for LKDTM tests to be a
>>>>> system exception, so doing "BUG" on a failure is actually against the
>>>>> design. So, really, a test harness needs to know to check dmesg for the
>>>>> results here. It almost looks like this check shouldn't live in LKDTM,
>>>>> but since it feels like other LKDTM tests, I'm happy to keep it there
>>>>> for now.
>>>>
>>>> Do you mean that you will apply this patch?
>>>
>>> Sorry for my confusing reply! I meant that I don't want to apply the
>>> patch, but I'm find to leave the stackleak check in LKDTM.
>>
>> Kees, I think I see a solution.
>>
>> Would you agree if I use dump_stack() instead of BUG() in case of test failure?
>> That would provide enough info for debugging and would NOT break your design.
> 
> I would be fine with that, yes! :)

Thank you! I'll send the v2 shortly.

Best regards,
Alexander