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

Commit Message

Dmitry Vyukov Dec. 13, 2016, 8:58 a.m. UTC

On Tue, Dec 13, 2016 at 6:57 AM, Maninder Singh <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Maninder Singh <[e-mail]>
> Reviewed-by: Ajeet Yadav <[e-mail]>
> ---
> 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:


the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andrey Ryabinin Dec. 13, 2016, 9:20 a.m. UTC | #1

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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Vyukov Dec. 13, 2016, 9:38 a.m. UTC | #2

On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
<[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrey Ryabinin Dec. 13, 2016, 1:59 p.m. UTC | #3

On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
> <[e-mail]> 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. 



 
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Vyukov Dec. 13, 2016, 2:13 p.m. UTC | #4

On Tue, Dec 13, 2016 at 2:59 PM, Andrey Ryabinin
<[e-mail]> wrote:
> On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
>> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
>> <[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrey Ryabinin Dec. 13, 2016, 3:29 p.m. UTC | #5

On 12/13/2016 05:13 PM, Dmitry Vyukov wrote:
> On Tue, Dec 13, 2016 at 2:59 PM, Andrey Ryabinin
> <[e-mail]> wrote:
>> On 12/13/2016 12:38 PM, Dmitry Vyukov wrote:
>>> On Tue, Dec 13, 2016 at 10:20 AM, Andrey Ryabinin
>>> <[e-mail]> 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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Vyukov Dec. 16, 2016, 10:46 a.m. UTC | #6

On Fri, Dec 16, 2016 at 11:29 AM, Vaneet Narang <[e-mail]> 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"
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in