[PATCH v2] kasan: Support for r/w instrumentation control

[PATCH v2] kasan: Support for r/w instrumentation control

[Maninder Singh]

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

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Dmitry Vyukov]

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

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Andrey Ryabinin]

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
> 

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Dmitry Vyukov]

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.

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Andrey Ryabinin]

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. 



 

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Dmitry Vyukov]

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.

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Andrey Ryabinin]

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.

Re: [PATCH v2] kasan: Support for r/w instrumentation control

[Dmitry Vyukov]

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"