Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1239437 > unrolled thread

[PATCH] x86/process: Silence KASAN warnings in get_wchan()

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-05 12:40 +0200
    Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-10-05 13:20 +0200
    Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Ingo Molnar <mingo@kernel.org> - 2015-10-05 13:30 +0200
      Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Dmitry Vyukov <dvyukov@google.com> - 2015-10-05 13:50 +0200
        Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-05 13:50 +0200
          Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Borislav Petkov <bp@alien8.de> - 2015-10-05 15:20 +0200
            Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andy Lutomirski <luto@amacapital.net> - 2015-10-05 20:50 +0200
          Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andi Kleen <ak@linux.intel.com> - 2015-10-05 18:40 +0200
            Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andrey Ryabinin <aryabinin@virtuozzo.com> - 2015-10-05 19:00 +0200
              Re: [PATCH] x86/process: Silence KASAN warnings in get_wchan() Andi Kleen <ak@linux.intel.com> - 2015-10-05 21:00 +0200

#1239437 — [PATCH] x86/process: Silence KASAN warnings in get_wchan()

get_wchan() is racy by design, it may access volatile stack
of running task, thus it may access redzone in a stack frame
and cause KASAN to warn about this.

Use kasan_disable_current()/kasan_enable_current() to silence
these warnings.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---

 Perhaps it would be better to add something like this:
	READ_ONCE_NOCHECK()
	{
		kasan_disable_current();
		READ_ONCE();
		kasan_enable_current();
	}
  ?

 arch/x86/kernel/process.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 39e585a..0488eb9 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -13,6 +13,7 @@
 #include <linux/random.h>
 #include <linux/user-return-notifier.h>
 #include <linux/dmi.h>
+#include <linux/kasan.h>
 #include <linux/utsname.h>
 #include <linux/stackprotector.h>
 #include <linux/tick.h>
@@ -514,7 +515,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
  */
 unsigned long get_wchan(struct task_struct *p)
 {
-	unsigned long start, bottom, top, sp, fp, ip;
+	unsigned long start, bottom, top, sp, fp, ip, ret = 0;
 	int count = 0;
 
 	if (!p || p == current || p->state == TASK_RUNNING)
@@ -550,14 +551,21 @@ unsigned long get_wchan(struct task_struct *p)
 	if (sp < bottom || sp > top)
 		return 0;
 
+	kasan_disable_current();
 	fp = READ_ONCE(*(unsigned long *)sp);
 	do {
 		if (fp < bottom || fp > top)
-			return 0;
+			goto out;
+
 		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
-		if (!in_sched_functions(ip))
-			return ip;
+		if (!in_sched_functions(ip)) {
+			ret = ip;
+			goto out;
+		}
 		fp = READ_ONCE(*(unsigned long *)fp);
 	} while (count++ < 16 && p->state != TASK_RUNNING);
-	return 0;
+
+out:
+	kasan_enable_current();
+	return ret;
 }
-- 
2.4.9

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1239468

On Mon, Oct 05, 2015 at 01:28:26PM +0300, Andrey Ryabinin wrote:
> get_wchan() is racy by design, it may access volatile stack
> of running task, thus it may access redzone in a stack frame
> and cause KASAN to warn about this.
> 
> Use kasan_disable_current()/kasan_enable_current() to silence
> these warnings.

So we're going to be sprinkling those around code which kasan doesn't
like? Can't we do better, without touching all code everywhere?

Probably not but let me ask that just in case...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239473

* Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:

> get_wchan() is racy by design, it may access volatile stack
> of running task, thus it may access redzone in a stack frame
> and cause KASAN to warn about this.
> 
> Use kasan_disable_current()/kasan_enable_current() to silence
> these warnings.
> 
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> ---
> 
>  Perhaps it would be better to add something like this:
> 	READ_ONCE_NOCHECK()
> 	{
> 		kasan_disable_current();
> 		READ_ONCE();
> 		kasan_enable_current();
> 	}
>   ?
> 
>  arch/x86/kernel/process.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 39e585a..0488eb9 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -13,6 +13,7 @@
>  #include <linux/random.h>
>  #include <linux/user-return-notifier.h>
>  #include <linux/dmi.h>
> +#include <linux/kasan.h>
>  #include <linux/utsname.h>
>  #include <linux/stackprotector.h>
>  #include <linux/tick.h>
> @@ -514,7 +515,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>   */
>  unsigned long get_wchan(struct task_struct *p)
>  {
> -	unsigned long start, bottom, top, sp, fp, ip;
> +	unsigned long start, bottom, top, sp, fp, ip, ret = 0;
>  	int count = 0;
>  
>  	if (!p || p == current || p->state == TASK_RUNNING)
> @@ -550,14 +551,21 @@ unsigned long get_wchan(struct task_struct *p)
>  	if (sp < bottom || sp > top)
>  		return 0;
>  
> +	kasan_disable_current();
>  	fp = READ_ONCE(*(unsigned long *)sp);
>  	do {
>  		if (fp < bottom || fp > top)
> -			return 0;
> +			goto out;

a break would do just fine too.

> +
>  		ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
> -		if (!in_sched_functions(ip))
> -			return ip;
> +		if (!in_sched_functions(ip)) {
> +			ret = ip;
> +			goto out;

ditto.

> +		}
>  		fp = READ_ONCE(*(unsigned long *)fp);
>  	} while (count++ < 16 && p->state != TASK_RUNNING);
> -	return 0;
> +
> +out:

and then the label would not be needed.

> +	kasan_enable_current();
> +	return ret;

But that's all pretty disgusting really.

Cannot we do better, such as annotating the function and then KASAN sorting out 
its false positives, or something like that?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239483

On Mon, Oct 5, 2015 at 1:23 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
>> get_wchan() is racy by design, it may access volatile stack
>> of running task, thus it may access redzone in a stack frame
>> and cause KASAN to warn about this.
>>
>> Use kasan_disable_current()/kasan_enable_current() to silence
>> these warnings.
>>
>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>> Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> ---
>>
>>  Perhaps it would be better to add something like this:
>>       READ_ONCE_NOCHECK()
>>       {
>>               kasan_disable_current();
>>               READ_ONCE();
>>               kasan_enable_current();
>>       }
>>   ?
>>
>>  arch/x86/kernel/process.c | 18 +++++++++++++-----
>>  1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>> index 39e585a..0488eb9 100644
>> --- a/arch/x86/kernel/process.c
>> +++ b/arch/x86/kernel/process.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/random.h>
>>  #include <linux/user-return-notifier.h>
>>  #include <linux/dmi.h>
>> +#include <linux/kasan.h>
>>  #include <linux/utsname.h>
>>  #include <linux/stackprotector.h>
>>  #include <linux/tick.h>
>> @@ -514,7 +515,7 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
>>   */
>>  unsigned long get_wchan(struct task_struct *p)
>>  {
>> -     unsigned long start, bottom, top, sp, fp, ip;
>> +     unsigned long start, bottom, top, sp, fp, ip, ret = 0;
>>       int count = 0;
>>
>>       if (!p || p == current || p->state == TASK_RUNNING)
>> @@ -550,14 +551,21 @@ unsigned long get_wchan(struct task_struct *p)
>>       if (sp < bottom || sp > top)
>>               return 0;
>>
>> +     kasan_disable_current();
>>       fp = READ_ONCE(*(unsigned long *)sp);
>>       do {
>>               if (fp < bottom || fp > top)
>> -                     return 0;
>> +                     goto out;
>
> a break would do just fine too.
>
>> +
>>               ip = READ_ONCE(*(unsigned long *)(fp + sizeof(unsigned long)));
>> -             if (!in_sched_functions(ip))
>> -                     return ip;
>> +             if (!in_sched_functions(ip)) {
>> +                     ret = ip;
>> +                     goto out;
>
> ditto.
>
>> +             }
>>               fp = READ_ONCE(*(unsigned long *)fp);
>>       } while (count++ < 16 && p->state != TASK_RUNNING);
>> -     return 0;
>> +
>> +out:
>
> and then the label would not be needed.
>
>> +     kasan_enable_current();
>> +     return ret;
>
> But that's all pretty disgusting really.
>
> Cannot we do better, such as annotating the function and then KASAN sorting out
> its false positives, or something like that?


We also plug __attribute__((no_sanitize_address)) on the function.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239484

On 10/05/2015 02:39 PM, Dmitry Vyukov wrote:
>>
>> But that's all pretty disgusting really.
>>
>> Cannot we do better, such as annotating the function and then KASAN sorting out
>> its false positives, or something like that?
> 
> 
> We also plug __attribute__((no_sanitize_address)) on the function.
> 

It's absolutely unusable:( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
If we add it here, it won't built because of: '__always_inline __read_once_size()'

But, I think I have the solution.
We could have some blacklist - list of function names which we should be ignored.
In kasan_report() we could resolve return address to function name and compare it with name in list.
If name in list -> ignore report.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239545

On Mon, Oct 05, 2015 at 02:46:30PM +0300, Andrey Ryabinin wrote:
> It's absolutely unusable:( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> If we add it here, it won't built because of: '__always_inline __read_once_size()'
> 
> But, I think I have the solution.
> We could have some blacklist - list of function names which we should be ignored.
> In kasan_report() we could resolve return address to function name and compare it with name in list.
> If name in list -> ignore report.

Sounds better. :)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239837

On Mon, Oct 5, 2015 at 6:15 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, Oct 05, 2015 at 02:46:30PM +0300, Andrey Ryabinin wrote:
>> It's absolutely unusable:( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>> If we add it here, it won't built because of: '__always_inline __read_once_size()'
>>
>> But, I think I have the solution.
>> We could have some blacklist - list of function names which we should be ignored.
>> In kasan_report() we could resolve return address to function name and compare it with name in list.
>> If name in list -> ignore report.
>
> Sounds better. :)

These ought to be rare.  probe_kernel_xyz are hopefully already
covered, which leaves strange accesses to things that are known to be
out of bounds, which ought to be just the memory allocation stuff
(already covered?) and stack walking code (this stuff).

I think that READ_ONCE_NOCHECK is a decent solution that isn't too messy.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239730

> But, I think I have the solution.
> We could have some blacklist - list of function names which we should be ignored.
> In kasan_report() we could resolve return address to function name and compare it with name in list.
> If name in list -> ignore report.

I think annotating statements is cleaner than functions, even if it
is more code. Much better documentation

But if you really want to annotate on the function level:

It's better to annotate the function directly than some hidden away list.
This way there is some indication that there are races in there, which is
generally useful documentation.

__racy_function or similar.

Also central lists are generally annoying as they cause patch conflicts.

If disabling with an attribute doesn't work, you could put it into a special section
with __attribute__((section ...)) and check the start/end symbol before reporting. 
That's how kprobes solves similar issues. It also has the advantage
that it stops inlining.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239748

On 10/05/2015 07:39 PM, Andi Kleen wrote:
>> But, I think I have the solution.
>> We could have some blacklist - list of function names which we should be ignored.
>> In kasan_report() we could resolve return address to function name and compare it with name in list.
>> If name in list -> ignore report.
> 
> I think annotating statements is cleaner than functions, even if it
> is more code. Much better documentation
> 

I agree with that, that's why I suggested to add READ_ONCE_NOCHECK():
	READ_ONCE_NOCHECK()
	{
		kasan_disable_current();
		READ_ONCE();
		kasan_enable_current();
	}

Anywone objects?

> But if you really want to annotate on the function level:
> 
> It's better to annotate the function directly than some hidden away list.
> This way there is some indication that there are races in there, which is
> generally useful documentation.
> 
> __racy_function or similar.
> 
> Also central lists are generally annoying as they cause patch conflicts.
> 
> If disabling with an attribute doesn't work, you could put it into a special section
> with __attribute__((section ...)) and check the start/end symbol before reporting. 
> That's how kprobes solves similar issues. It also has the advantage
> that it stops inlining.


Yes, it might be better. Although, because of broken -fconserve-stack, this may
not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533
Function splitter may split original function into two parts and it always puts one split
part in default .text section.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239843

> Yes, it might be better. Although, because of broken -fconserve-stack, this may
> not work in some cases - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63533
> Function splitter may split original function into two parts and it always puts one split
> part in default .text section.

Interesting.

I guess could just add noinline too. It only happens with inlining, right?

Probably that needs to be added to all the existing section users too,
like __kprobes.

-Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web