2016-12-12 09:38:08

by Maninder Singh

[permalink] [raw]
Subject: [PATCH 1/1] kasan: Support for r/w instrumentation control

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 <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
Reviewed-by: Ajeet Yadav <[email protected]>
---
lib/Kconfig.kasan | 16 ++++++++++++++++
scripts/Makefile.kasan | 4 ++++
2 files changed, 20 insertions(+)

diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
index bd38aab..6f0f774 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


2016-12-12 10:06:01

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: [PATCH 1/1] kasan: Support for r/w instrumentation control

On Mon, Dec 12, 2016 at 10:32 AM, Maninder Singh
<[email protected]> 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.

Hi,

Do you actually hit an issue with image size? In what context?
Do you use inline/outline instrumentation? Does switching to the other
option help?
Does it make sense to ever disable writes? I assume that you are
disabling reads, right?
Disabling both certainly does not make sense.


> Signed-off-by: Vaneet narang <[email protected]>
> Signed-off-by: Maninder Singh <[email protected]>
> Reviewed-by: Ajeet Yadav <[email protected]>
> ---
> lib/Kconfig.kasan | 16 ++++++++++++++++
> scripts/Makefile.kasan | 4 ++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index bd38aab..6f0f774 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.

/\/\/\

double space in "of memory"

> +
> +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
>

2016-12-12 10:29:22

by Vaneet Narang

[permalink] [raw]
Subject: RE: Re: [PATCH 1/1] kasan: Support for r/w instrumentation control

 Hi,

> Do you actually hit an issue with image size? In what context?
> Do you use inline/outline instrumentation? Does switching to the other
> option help?

Memory access with KASAN enabled Image has overhead in terms of cpu execution.
Sometimes we are not able to reproduce race condition issues with these overhead in
place. So user should have control atleast over read instrumentation.

> Does it make sense to ever disable writes? I assume that you are

Write instrumentation control is majorly kept to be inline with ASAN for user space
applications.
Also write is sometimes useful when uImage is already sanitized and some corruption
is done by kernel modules by doing some direct memory access then both read / write sanity of uImage
can be avoided.

> disabling reads, right?
> Disabling both certainly does not make sense.

Regards,
Vaneet Narang

2016-12-12 10:49:40

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: Re: [PATCH 1/1] kasan: Support for r/w instrumentation control

On Mon, Dec 12, 2016 at 11:29 AM, Vaneet Narang <[email protected]> wrote:
> Hi,
>
>> Do you actually hit an issue with image size? In what context?
>> Do you use inline/outline instrumentation? Does switching to the other
>> option help?
>
> Memory access with KASAN enabled Image has overhead in terms of cpu execution.
> Sometimes we are not able to reproduce race condition issues with these overhead in
> place. So user should have control atleast over read instrumentation.

Don't you want to disable KASAN entirely in such case?


>> Does it make sense to ever disable writes? I assume that you are
>
> Write instrumentation control is majorly kept to be inline with ASAN for user space
> applications.
> Also write is sometimes useful when uImage is already sanitized and some corruption
> is done by kernel modules by doing some direct memory access then both read / write sanity of uImage
> can be avoided.

But then you don't need KASAN at all.


>> disabling reads, right?
>> Disabling both certainly does not make sense.

2016-12-12 12:05:15

by Vaneet Narang

[permalink] [raw]
Subject: Re: Re: [PATCH 1/1] kasan: Support for r/w instrumentation control

Hi,

>>> Do you actually hit an issue with image size? In what context?
>>> Do you use inline/outline instrumentation? Does switching to the other
>>> option help?
>>
>> Memory access with KASAN enabled Image has overhead in terms of cpu execution.
>> Sometimes we are not able to reproduce race condition issues with these overhead in
>> place. So user should have control atleast over read instrumentation.
>
>Don't you want to disable KASAN entirely in such case?

hmmm, but we need KASAN to detect corruption issues so overhead can be
reduced by switching OFF read instrumentation. Generally Reads are much more frequent
than writes as latest arm64 kernel has 224000 reads and 62300 writes
which is almost 3.5 times. So i think this control is required.


>>> Does it make sense to ever disable writes? I assume that you are
>>
>> Write instrumentation control is majorly kept to be inline with ASAN for user space
>> applications.
>> Also write is sometimes useful when uImage is already sanitized and some corruption
>> is done by kernel modules by doing some direct memory access then both read / write sanity of uImage
>> can be avoided.
>
>But then you don't need KASAN at all.

KASAN support is required in this case to detect module issues.
KASAN provides asan_load / asan_store definition as these functions
are added by compiler in modules before every memory access.
These are the functions which will do address sanity and detect errors. 

Regards,
Vaneet Narang

2016-12-12 13:45:22

by Dmitry Vyukov

[permalink] [raw]
Subject: Re: Re: [PATCH 1/1] kasan: Support for r/w instrumentation control

On Mon, Dec 12, 2016 at 2:43 PM, Dmitry Vyukov <[email protected]> wrote:
>
> On Mon, Dec 12, 2016 at 12:12 PM, Vaneet Narang <[email protected]> wrote:
>>
>> Hi,
>>
>> >>> Do you actually hit an issue with image size? In what context?
>> >>> Do you use inline/outline instrumentation? Does switching to the other
>> >>> option help?
>> >>
>> >> Memory access with KASAN enabled Image has overhead in terms of cpu execution.
>> >> Sometimes we are not able to reproduce race condition issues with these overhead in
>> >> place. So user should have control atleast over read instrumentation.
>> >
>> >Don't you want to disable KASAN entirely in such case?
>>
>> hmmm, but we need KASAN to detect corruption issues so overhead can be
>> reduced by switching OFF read instrumentation. Generally Reads are much more frequent
>> than writes as latest arm64 kernel has 224000 reads and 62300 writes
>> which is almost 3.5 times. So i think this control is required.
>>
>>
>> >>> Does it make sense to ever disable writes? I assume that you are
>> >>
>> >> Write instrumentation control is majorly kept to be inline with ASAN for user space
>> >> applications.
>> >> Also write is sometimes useful when uImage is already sanitized and some corruption
>> >> is done by kernel modules by doing some direct memory access then both read / write sanity of uImage
>> >> can be avoided.
>> >
>> >But then you don't need KASAN at all.
>>
>> KASAN support is required in this case to detect module issues.
>> KASAN provides asan_load / asan_store definition as these functions
>> are added by compiler in modules before every memory access.
>> These are the functions which will do address sanity and detect errors.
>

[resending as plain text]

Ah, OK, I see. So you want to e.g. not instrument kernel (for both
reads and writes), but instrument a single module. That makes sense.
Please extend the Usage section of KASAN docs:
https://kernel.org/doc/html/latest/dev-tools/kasan.html#usage
mention not instrumenting writes for whole kernel, and instrumenting a
single module.