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

[PATCH 1/1] 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>
---
 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

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

[Dmitry Vyukov]

On Mon, Dec 12, 2016 at 10:32 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.

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 <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
> ---
>  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
>

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

[Vaneet Narang]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

 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

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

[Dmitry Vyukov]

On Mon, Dec 12, 2016 at 11:29 AM, Vaneet Narang <v.narang@samsung.com> 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.

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

[Vaneet Narang]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

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

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

[Dmitry Vyukov]

On Mon, Dec 12, 2016 at 2:43 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Dec 12, 2016 at 12:12 PM, Vaneet Narang <v.narang@samsung.com> 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.