All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] kasan: Support for r/w instrumentation control
@ 2016-12-13  5:57 Maninder Singh
  2016-12-13  8:58 ` Dmitry Vyukov
  0 siblings, 1 reply; 11+ messages in thread
From: Maninder Singh @ 2016-12-13  5:57 UTC (permalink / raw)
  To: aryabinin, glider, dvyukov, corbet, mmarek, akpm
  Cc: kasan-dev, linux-doc, linux-kernel, linux-kbuild, pankaj.m,
	ajeet.y, Maninder Singh, Vaneet narang

This provide option to control sanity of read and write operations
Both read and write instrumentation increase size of uImage, So using
this option read or write instrumentation can be avoided if not required.
Useful in case of module sanity, using this uImage sanity can be avoided.

Also user space ASAN provides this support for read/write instrumentation
control.

Signed-off-by: Vaneet narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
---
v1 -> v2: Added Documentation for the same.

 Documentation/dev-tools/kasan.rst | 16 ++++++++++++++++
 lib/Kconfig.kasan                 | 16 ++++++++++++++++
 scripts/Makefile.kasan            |  4 ++++
 3 files changed, 36 insertions(+)

diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst
index f7a18f2..b8147df 100644
--- a/Documentation/dev-tools/kasan.rst
+++ b/Documentation/dev-tools/kasan.rst
@@ -40,6 +40,22 @@ similar to the following to the respective kernel Makefile:
 
     KASAN_SANITIZE := n
 
+Control Over Read/Write Instrumentation of kernel::
+
+- To Disable Read Instrumentation of kernel with:
+
+    CONFIG_KASAN_READS = n
+
+Because in some cases we need to check only memory write sanitization
+for better performance, read instrumentation can be disabled.
+
+- To Disable Write Instrumentation of kernel with:
+
+    CONFIG_KASAN_WRITES = n
+
+In case when to instrument only external modules, not the entire kernel
+for read or write intrumentation or both.
+
 Error reports
 ~~~~~~~~~~~~~
 
diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index bd38aab..37d1de9 100644
--- a/lib/Kconfig.kasan
+++ b/lib/Kconfig.kasan
@@ -45,6 +45,22 @@ config KASAN_INLINE
 
 endchoice
 
+config KASAN_READS
+	prompt "Read instrumentation"
+	bool
+	default y
+	depends on KASAN
+	help
+	  This configuration controls the sanity of memory read.
+
+config KASAN_WRITES
+	prompt "Write instrumentation"
+	bool
+	default y
+	depends on KASAN
+	help
+	  This configuration controls the sanity of memory write.
+
 config TEST_KASAN
 	tristate "Module for testing kasan for bug detection"
 	depends on m && KASAN
diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
index 37323b0..a61b18e 100644
--- a/scripts/Makefile.kasan
+++ b/scripts/Makefile.kasan
@@ -29,3 +29,7 @@ else
     endif
 endif
 endif
+
+CFLAGS_KASAN += $(call cc-option, \
+		$(if $(CONFIG_KASAN_READS),, --param asan-instrument-reads=0) \
+		$(if $(CONFIG_KASAN_WRITES),, --param asan-instrument-writes=0))
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
  2016-12-13  5:57 [PATCH v2] kasan: Support for r/w instrumentation control Maninder Singh
@ 2016-12-13  8:58 ` Dmitry Vyukov
  2016-12-13  9:20     ` Andrey Ryabinin
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Vyukov @ 2016-12-13  8:58 UTC (permalink / raw)
  To: Maninder Singh
  Cc: Andrey Ryabinin, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang

On Tue, Dec 13, 2016 at 6:57 AM, Maninder Singh <maninder1.s@samsung.com> wrote:
> This provide option to control sanity of read and write operations
> Both read and write instrumentation increase size of uImage, So using
> this option read or write instrumentation can be avoided if not required.
> Useful in case of module sanity, using this uImage sanity can be avoided.
>
> Also user space ASAN provides this support for read/write instrumentation
> control.
>
> Signed-off-by: Vaneet narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
> ---
> v1 -> v2: Added Documentation for the same.
>
>  Documentation/dev-tools/kasan.rst | 16 ++++++++++++++++
>  lib/Kconfig.kasan                 | 16 ++++++++++++++++
>  scripts/Makefile.kasan            |  4 ++++
>  3 files changed, 36 insertions(+)
>
> diff --git a/Documentation/dev-tools/kasan.rst b/Documentation/dev-tools/kasan.rst
> index f7a18f2..b8147df 100644
> --- a/Documentation/dev-tools/kasan.rst
> +++ b/Documentation/dev-tools/kasan.rst
> @@ -40,6 +40,22 @@ similar to the following to the respective kernel Makefile:
>
>      KASAN_SANITIZE := n
>
> +Control Over Read/Write Instrumentation of kernel::

double colon at the end of line

> +
> +- To Disable Read Instrumentation of kernel with:

Strange choice of capital letters.

> +
> +    CONFIG_KASAN_READS = n

Are configs ever set to = n? I can't find any cases in my .config file.
I thought configs are disabled with:

# CONFIG_KASAN_READS is not set

> +
> +Because in some cases we need to check only memory write sanitization
> +for better performance, read instrumentation can be disabled.
> +
> +- To Disable Write Instrumentation of kernel with:

I am not a native speaker but this looks malformed.
I would say either "Disable write instrumentation of kernel with" or
"To disable write instrumentation of kernel set"

> +    CONFIG_KASAN_WRITES = n
> +
> +In case when to instrument only external modules, not the entire kernel
> +for read or write intrumentation or both.
> +

I propose something along these lines:


--- a/Documentation/dev-tools/kasan.rst
+++ b/Documentation/dev-tools/kasan.rst
@@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:

     KASAN_SANITIZE := n

+Sometimes it may be useful to disable instrumentation of reads, or writes
+or both for the entire kernel. For example, if binary size is a concern,
+it may be useful to disable instrumentation of reads to reduce binary size but
+still catch more harmful bugs on writes. Or, if one is interested only in
+sanitization of a particular module and performance is a concern, she can
+disable instrumentation of both reads and writes for kernel code.
+Instrumentation can be disabled with CONFIG_KASAN_READS and
CONFIG_KASAN_WRITES.
+
 Error reports

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
  2016-12-13  8:58 ` Dmitry Vyukov
@ 2016-12-13  9:20     ` Andrey Ryabinin
  0 siblings, 0 replies; 11+ messages in thread
From: Andrey Ryabinin @ 2016-12-13  9:20 UTC (permalink / raw)
  To: Dmitry Vyukov, Maninder Singh
  Cc: Alexander Potapenko, Jonathan Corbet, Michal Marek,
	Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang



On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:

> --- a/Documentation/dev-tools/kasan.rst
> +++ b/Documentation/dev-tools/kasan.rst
> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
> 
>      KASAN_SANITIZE := n
> 
> +Sometimes it may be useful to disable instrumentation of reads, or writes
> +or both for the entire kernel. For example, if binary size is a concern,
> +it may be useful to disable instrumentation of reads to reduce binary size but
> +still catch more harmful bugs on writes. Or, if one is interested only in
> +sanitization of a particular module and performance is a concern, she can
> +disable instrumentation of both reads and writes for kernel code.
> +Instrumentation can be disabled with CONFIG_KASAN_READS and
> CONFIG_KASAN_WRITES.
> +

I don't understand this. How this can be related to modules? Configs are global.
You can't just disable/enable config per module.

>  Error reports
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
@ 2016-12-13  9:20     ` Andrey Ryabinin
  0 siblings, 0 replies; 11+ messages in thread
From: Andrey Ryabinin @ 2016-12-13  9:20 UTC (permalink / raw)
  To: Dmitry Vyukov, Maninder Singh
  Cc: Alexander Potapenko, Jonathan Corbet, Michal Marek,
	Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang



On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:

> --- a/Documentation/dev-tools/kasan.rst
> +++ b/Documentation/dev-tools/kasan.rst
> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
> 
>      KASAN_SANITIZE := n
> 
> +Sometimes it may be useful to disable instrumentation of reads, or writes
> +or both for the entire kernel. For example, if binary size is a concern,
> +it may be useful to disable instrumentation of reads to reduce binary size but
> +still catch more harmful bugs on writes. Or, if one is interested only in
> +sanitization of a particular module and performance is a concern, she can
> +disable instrumentation of both reads and writes for kernel code.
> +Instrumentation can be disabled with CONFIG_KASAN_READS and
> CONFIG_KASAN_WRITES.
> +

I don't understand this. How this can be related to modules? Configs are global.
You can't just disable/enable config per module.

>  Error reports
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
  2016-12-13  9:20     ` Andrey Ryabinin
  (?)
@ 2016-12-13  9:38     ` Dmitry Vyukov
  2016-12-13 13:59         ` Andrey Ryabinin
  -1 siblings, 1 reply; 11+ messages in thread
From: Dmitry Vyukov @ 2016-12-13  9:38 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Maninder Singh, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang

On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:
>
>> --- a/Documentation/dev-tools/kasan.rst
>> +++ b/Documentation/dev-tools/kasan.rst
>> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
>>
>>      KASAN_SANITIZE := n
>>
>> +Sometimes it may be useful to disable instrumentation of reads, or writes
>> +or both for the entire kernel. For example, if binary size is a concern,
>> +it may be useful to disable instrumentation of reads to reduce binary size but
>> +still catch more harmful bugs on writes. Or, if one is interested only in
>> +sanitization of a particular module and performance is a concern, she can
>> +disable instrumentation of both reads and writes for kernel code.
>> +Instrumentation can be disabled with CONFIG_KASAN_READS and
>> CONFIG_KASAN_WRITES.
>> +
>
> I don't understand this. How this can be related to modules? Configs are global.
> You can't just disable/enable config per module.


Build everything without instrumentation. Then enable instrumentation
and do "make lib/test_kasan.ko".
Or build everything, copy out bzImage, change config, build everything again.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
  2016-12-13  9:38     ` Dmitry Vyukov
@ 2016-12-13 13:59         ` Andrey Ryabinin
  0 siblings, 0 replies; 11+ messages in thread
From: Andrey Ryabinin @ 2016-12-13 13:59 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Maninder Singh, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang

On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:
>>
>>> --- a/Documentation/dev-tools/kasan.rst
>>> +++ b/Documentation/dev-tools/kasan.rst
>>> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
>>>
>>>      KASAN_SANITIZE := n
>>>
>>> +Sometimes it may be useful to disable instrumentation of reads, or writes
>>> +or both for the entire kernel. For example, if binary size is a concern,
>>> +it may be useful to disable instrumentation of reads to reduce binary size but
>>> +still catch more harmful bugs on writes. Or, if one is interested only in
>>> +sanitization of a particular module and performance is a concern, she can
>>> +disable instrumentation of both reads and writes for kernel code.
>>> +Instrumentation can be disabled with CONFIG_KASAN_READS and
>>> CONFIG_KASAN_WRITES.
>>> +
>>
>> I don't understand this. How this can be related to modules? Configs are global.
>> You can't just disable/enable config per module.
> 
> 
> Build everything without instrumentation. Then enable instrumentation
> and do "make lib/test_kasan.ko".
> Or build everything, copy out bzImage, change config, build everything again.

Yeah, this is soooooo convenient...

Seriously speaking, per-file instrumentation is absolutely irrelevant to this patch and should have been
addressed from a different angle. E.g. see how UBSAN/GCOV/KCOV do that.

As for this patch, I'd say only one option would be enough - KASAN_DONT_SANITIZE_READS.
Nobody wants to sanitize only reads without writes, right? Writes are fewer and more dangerous. 



 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
@ 2016-12-13 13:59         ` Andrey Ryabinin
  0 siblings, 0 replies; 11+ messages in thread
From: Andrey Ryabinin @ 2016-12-13 13:59 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Maninder Singh, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang

On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>>
>> On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:
>>
>>> --- a/Documentation/dev-tools/kasan.rst
>>> +++ b/Documentation/dev-tools/kasan.rst
>>> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
>>>
>>>      KASAN_SANITIZE := n
>>>
>>> +Sometimes it may be useful to disable instrumentation of reads, or writes
>>> +or both for the entire kernel. For example, if binary size is a concern,
>>> +it may be useful to disable instrumentation of reads to reduce binary size but
>>> +still catch more harmful bugs on writes. Or, if one is interested only in
>>> +sanitization of a particular module and performance is a concern, she can
>>> +disable instrumentation of both reads and writes for kernel code.
>>> +Instrumentation can be disabled with CONFIG_KASAN_READS and
>>> CONFIG_KASAN_WRITES.
>>> +
>>
>> I don't understand this. How this can be related to modules? Configs are global.
>> You can't just disable/enable config per module.
> 
> 
> Build everything without instrumentation. Then enable instrumentation
> and do "make lib/test_kasan.ko".
> Or build everything, copy out bzImage, change config, build everything again.

Yeah, this is soooooo convenient...

Seriously speaking, per-file instrumentation is absolutely irrelevant to this patch and should have been
addressed from a different angle. E.g. see how UBSAN/GCOV/KCOV do that.

As for this patch, I'd say only one option would be enough - KASAN_DONT_SANITIZE_READS.
Nobody wants to sanitize only reads without writes, right? Writes are fewer and more dangerous. 



 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
  2016-12-13 13:59         ` Andrey Ryabinin
  (?)
@ 2016-12-13 14:13         ` Dmitry Vyukov
  2016-12-13 15:29             ` Andrey Ryabinin
       [not found]           ` <CGME20161213152904epcas3p15f600392fd9f54fe1f68ed9f87d9f03b@epcas3p1.samsung.com>
  -1 siblings, 2 replies; 11+ messages in thread
From: Dmitry Vyukov @ 2016-12-13 14:13 UTC (permalink / raw)
  To: Andrey Ryabinin
  Cc: Maninder Singh, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang

On Tue, Dec 13, 2016 at 2:59 PM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
>> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:
>>>
>>>
>>> On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:
>>>
>>>> --- a/Documentation/dev-tools/kasan.rst
>>>> +++ b/Documentation/dev-tools/kasan.rst
>>>> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
>>>>
>>>>      KASAN_SANITIZE := n
>>>>
>>>> +Sometimes it may be useful to disable instrumentation of reads, or writes
>>>> +or both for the entire kernel. For example, if binary size is a concern,
>>>> +it may be useful to disable instrumentation of reads to reduce binary size but
>>>> +still catch more harmful bugs on writes. Or, if one is interested only in
>>>> +sanitization of a particular module and performance is a concern, she can
>>>> +disable instrumentation of both reads and writes for kernel code.
>>>> +Instrumentation can be disabled with CONFIG_KASAN_READS and
>>>> CONFIG_KASAN_WRITES.
>>>> +
>>>
>>> I don't understand this. How this can be related to modules? Configs are global.
>>> You can't just disable/enable config per module.
>>
>>
>> Build everything without instrumentation. Then enable instrumentation
>> and do "make lib/test_kasan.ko".
>> Or build everything, copy out bzImage, change config, build everything again.
>
> Yeah, this is soooooo convenient...
>
> Seriously speaking, per-file instrumentation is absolutely irrelevant to this patch and should have been
> addressed from a different angle. E.g. see how UBSAN/GCOV/KCOV do that.


KASAN already has that functionality (i.e. KASAN_SANITIZE_main.o :=
n). But that functionality is intended for cases when we want to
persistently disable instrumentation of some files (e.g. if they cause
crashes of false positives). CONFIG_KASAN_READS/WRITES is intended for
situations when one wants to disable instrumentation wholesale.


> As for this patch, I'd say only one option would be enough - KASAN_DONT_SANITIZE_READS.
> Nobody wants to sanitize only reads without writes, right? Writes are fewer and more dangerous.

I've asked this question in v1. See the case related to modules -- one
can use completely uninstrumented kernel, but load an instrumented
modules.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
  2016-12-13 14:13         ` Dmitry Vyukov
@ 2016-12-13 15:29             ` Andrey Ryabinin
       [not found]           ` <CGME20161213152904epcas3p15f600392fd9f54fe1f68ed9f87d9f03b@epcas3p1.samsung.com>
  1 sibling, 0 replies; 11+ messages in thread
From: Andrey Ryabinin @ 2016-12-13 15:29 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Maninder Singh, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang



On 12/13/2016 05:13 PM, Dmitry Vyukov wrote:
> On Tue, Dec 13, 2016 at 2:59 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
>>> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
>>> <aryabinin@virtuozzo.com> wrote:
>>>>
>>>>
>>>> On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:
>>>>
>>>>> --- a/Documentation/dev-tools/kasan.rst
>>>>> +++ b/Documentation/dev-tools/kasan.rst
>>>>> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
>>>>>
>>>>>      KASAN_SANITIZE := n
>>>>>
>>>>> +Sometimes it may be useful to disable instrumentation of reads, or writes
>>>>> +or both for the entire kernel. For example, if binary size is a concern,
>>>>> +it may be useful to disable instrumentation of reads to reduce binary size but
>>>>> +still catch more harmful bugs on writes. Or, if one is interested only in
>>>>> +sanitization of a particular module and performance is a concern, she can
>>>>> +disable instrumentation of both reads and writes for kernel code.
>>>>> +Instrumentation can be disabled with CONFIG_KASAN_READS and
>>>>> CONFIG_KASAN_WRITES.
>>>>> +
>>>>
>>>> I don't understand this. How this can be related to modules? Configs are global.
>>>> You can't just disable/enable config per module.
>>>
>>>
>>> Build everything without instrumentation. Then enable instrumentation
>>> and do "make lib/test_kasan.ko".
>>> Or build everything, copy out bzImage, change config, build everything again.
>>
>> Yeah, this is soooooo convenient...
>>
>> Seriously speaking, per-file instrumentation is absolutely irrelevant to this patch and should have been
>> addressed from a different angle. E.g. see how UBSAN/GCOV/KCOV do that.
> 
> 
> KASAN already has that functionality (i.e. KASAN_SANITIZE_main.o :=
> n). But that functionality is intended for cases when we want to
> persistently disable instrumentation of some files (e.g. if they cause
> crashes of false positives). CONFIG_KASAN_READS/WRITES is intended for
> situations when one wants to disable instrumentation wholesale.
> 

I'm talking about UBSAN_SANITIZE_ALL/KCOV_INSTRUMENT_ALL/GCOV_PROFILE_ALL
KASAN doesn't have something similar. I didn't add this because IMO it's not very useful for KASAN.
One may have a bug in instrumented code, but it can be easily missed if access is done in generic
code. Very simple example is passing invalid pointer in strcpy()


> 
>> As for this patch, I'd say only one option would be enough - KASAN_DONT_SANITIZE_READS.
>> Nobody wants to sanitize only reads without writes, right? Writes are fewer and more dangerous.
> 
> I've asked this question in v1. See the case related to modules -- one
> can use completely uninstrumented kernel, but load an instrumented
> modules.
> 
I get it, but again, it's not the right way to address this problem.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
@ 2016-12-13 15:29             ` Andrey Ryabinin
  0 siblings, 0 replies; 11+ messages in thread
From: Andrey Ryabinin @ 2016-12-13 15:29 UTC (permalink / raw)
  To: Dmitry Vyukov
  Cc: Maninder Singh, Alexander Potapenko, Jonathan Corbet,
	Michal Marek, Andrew Morton, kasan-dev, linux-doc, LKML,
	open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav, Vaneet narang



On 12/13/2016 05:13 PM, Dmitry Vyukov wrote:
> On Tue, Dec 13, 2016 at 2:59 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
>>> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
>>> <aryabinin@virtuozzo.com> wrote:
>>>>
>>>>
>>>> On 12/13/2016 11:58 AM, Dmitry Vyukov wrote:
>>>>
>>>>> --- a/Documentation/dev-tools/kasan.rst
>>>>> +++ b/Documentation/dev-tools/kasan.rst
>>>>> @@ -40,6 +40,14 @@ similar to the following to the respective kernel Makefile:
>>>>>
>>>>>      KASAN_SANITIZE := n
>>>>>
>>>>> +Sometimes it may be useful to disable instrumentation of reads, or writes
>>>>> +or both for the entire kernel. For example, if binary size is a concern,
>>>>> +it may be useful to disable instrumentation of reads to reduce binary size but
>>>>> +still catch more harmful bugs on writes. Or, if one is interested only in
>>>>> +sanitization of a particular module and performance is a concern, she can
>>>>> +disable instrumentation of both reads and writes for kernel code.
>>>>> +Instrumentation can be disabled with CONFIG_KASAN_READS and
>>>>> CONFIG_KASAN_WRITES.
>>>>> +
>>>>
>>>> I don't understand this. How this can be related to modules? Configs are global.
>>>> You can't just disable/enable config per module.
>>>
>>>
>>> Build everything without instrumentation. Then enable instrumentation
>>> and do "make lib/test_kasan.ko".
>>> Or build everything, copy out bzImage, change config, build everything again.
>>
>> Yeah, this is soooooo convenient...
>>
>> Seriously speaking, per-file instrumentation is absolutely irrelevant to this patch and should have been
>> addressed from a different angle. E.g. see how UBSAN/GCOV/KCOV do that.
> 
> 
> KASAN already has that functionality (i.e. KASAN_SANITIZE_main.o :=
> n). But that functionality is intended for cases when we want to
> persistently disable instrumentation of some files (e.g. if they cause
> crashes of false positives). CONFIG_KASAN_READS/WRITES is intended for
> situations when one wants to disable instrumentation wholesale.
> 

I'm talking about UBSAN_SANITIZE_ALL/KCOV_INSTRUMENT_ALL/GCOV_PROFILE_ALL
KASAN doesn't have something similar. I didn't add this because IMO it's not very useful for KASAN.
One may have a bug in instrumented code, but it can be easily missed if access is done in generic
code. Very simple example is passing invalid pointer in strcpy()


> 
>> As for this patch, I'd say only one option would be enough - KASAN_DONT_SANITIZE_READS.
>> Nobody wants to sanitize only reads without writes, right? Writes are fewer and more dangerous.
> 
> I've asked this question in v1. See the case related to modules -- one
> can use completely uninstrumented kernel, but load an instrumented
> modules.
> 
I get it, but again, it's not the right way to address this problem.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] kasan: Support for r/w instrumentation control
       [not found]             ` <20161216102902epcms5p1acc3482dea59a0eb85523ed082a31841@epcms5p1>
@ 2016-12-16 10:46               ` Dmitry Vyukov
  0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Vyukov @ 2016-12-16 10:46 UTC (permalink / raw)
  To: Vaneet narang
  Cc: Andrey Ryabinin, Maninder Singh, Alexander Potapenko,
	Jonathan Corbet, Michal Marek, Andrew Morton, kasan-dev,
	linux-doc, LKML, open list:KERNEL BUILD + fi...,
	PANKAJ MISHRA, Ajeet Kumar Yadav

On Fri, Dec 16, 2016 at 11:29 AM, Vaneet Narang <v.narang@samsung.com> wrote:
>
> Hi Andrey,
>
>
> There are times when requirement is only to find write issues so user should have flexibility to
> skip read instrumentation for better performance with KASAN enabled build to find realtime
> issues as well.
>
> >> crashes of false positives). CONFIG_KASAN_READS/WRITES is intended for
> >> situations when one wants to disable instrumentation wholesale.
> >>
> >
> >I'm talking about UBSAN_SANITIZE_ALL/KCOV_INSTRUMENT_ALL/GCOV_PROFILE_ALL
> >KASAN doesn't have something similar. I didn't add this because IMO it's not very useful for KASAN.
> >One may have a bug in instrumented code, but it can be easily missed if access is done in generic
> >code. Very simple example is passing invalid pointer in strcpy()
>
>
> Old toolchains already add asan hooks before strcpy, strcmp. Please check assembly generated
> for module with below code using gcc 4.9.
>
> static noinline void __init test_kasan_module(void) {
>         strcpy(test, "testing strcpy");
> }
>
> 0000000000001108 <test_kasan_module>:
>     1108:       a9bd7bfd        stp     x29, x30, [sp,#-48]!
>     110c:       d28001e1        mov     x1, #0xf                        // #15
>     1110:       910003fd        mov     x29, sp
>     1114:       a90153f3        stp     x19, x20, [sp,#16]
>     1118:       58000253        ldr     x19, 1160 <test_kasan_module+0x58>
>     111c:       f90013f5        str     x21, [sp,#32]
>     1120:       aa1303e0        mov     x0, x19
>     1124:       94000000        bl      0 <__asan_loadN>              // Instrumented Read of size 15
>     1128:       58000215        ldr     x21, 1168 <test_kasan_module+0x60>
>     112c:       d28001e1        mov     x1, #0xf                        // #15
>     1130:       910042b4        add     x20, x21, #0x10
>     1134:       aa1403e0        mov     x0, x20
>     1138:       94000000        bl      0 <__asan_storeN>          // Instrumented Write of size 15
>     113c:       f9400260        ldr     x0, [x19]
>     1140:       f9000aa0        str     x0, [x21,#16]
>     1144:       f8407260        ldr     x0, [x19,#7]
>     1148:       f80172a0        str     x0, [x21,#23]
>     114c:       a94153f3        ldp     x19, x20, [sp,#16]
>
>
>
> Similar behaviour for strcmp, memset, memcpy ... but  with latest compiler 6.2,
> this implementation is removed from compiler in this case we can define wrappers
> in kasan.c for these function like we are already doing for memcpy, memmove, memset
>
>


One option would be to simply compile kernel as:
$ make CC="gcc --param asan-instrument-reads=0"

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-12-16 10:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-13  5:57 [PATCH v2] kasan: Support for r/w instrumentation control Maninder Singh
2016-12-13  8:58 ` Dmitry Vyukov
2016-12-13  9:20   ` Andrey Ryabinin
2016-12-13  9:20     ` Andrey Ryabinin
2016-12-13  9:38     ` Dmitry Vyukov
2016-12-13 13:59       ` Andrey Ryabinin
2016-12-13 13:59         ` Andrey Ryabinin
2016-12-13 14:13         ` Dmitry Vyukov
2016-12-13 15:29           ` Andrey Ryabinin
2016-12-13 15:29             ` Andrey Ryabinin
     [not found]           ` <CGME20161213152904epcas3p15f600392fd9f54fe1f68ed9f87d9f03b@epcas3p1.samsung.com>
     [not found]             ` <20161216102902epcms5p1acc3482dea59a0eb85523ed082a31841@epcms5p1>
2016-12-16 10:46               ` Dmitry Vyukov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.