[PATCH 0/2] KASAN: fix a type conversion error and add test

[PATCH 0/2] KASAN: fix a type conversion error and add test

[Wang Long]

Hi,

This patchset fix a type conversion error for KASAN.

patch 1: this patch add some out-of-bounds testcases, the current KASAN code
can not find these bugs.

patch 2: fix the type conversion error, with this patch, KASAN could find
these out-of-bounds bugs.

Wang Long (2):
  lib: test_kasan: add some testcases
  kasan: Fix a type conversion error

 lib/test_kasan.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/kasan/kasan.c |  2 +-
 2 files changed, 70 insertions(+), 1 deletion(-)

-- 
1.8.3.4

[PATCH 1/2] lib: test_kasan: add some testcases

[Wang Long]

This patch add some out of bounds testcases to test_kasan
module.

Signed-off-by: Wang Long <long.wanglong@huawei.com>
---
 lib/test_kasan.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/lib/test_kasan.c b/lib/test_kasan.c
index c1efb1b..c32f3b0 100644
--- a/lib/test_kasan.c
+++ b/lib/test_kasan.c
@@ -138,6 +138,71 @@ static noinline void __init kmalloc_oob_16(void)
 	kfree(ptr2);
 }
 
+static noinline void __init kmalloc_oob_memset_2(void)
+{
+	char *ptr;
+	size_t size = 8;
+
+	pr_info("out-of-bounds in memset2\n");
+	ptr = kmalloc(size, GFP_KERNEL);
+	if (!ptr) {
+		pr_err("Allocation failed\n");
+		return;
+	}
+
+	memset(ptr+7, 0, 2);
+	kfree(ptr);
+}
+
+static noinline void __init kmalloc_oob_memset_4(void)
+{
+	char *ptr;
+	size_t size = 8;
+
+	pr_info("out-of-bounds in memset4\n");
+	ptr = kmalloc(size, GFP_KERNEL);
+	if (!ptr) {
+		pr_err("Allocation failed\n");
+		return;
+	}
+
+	memset(ptr+5, 0, 4);
+	kfree(ptr);
+}
+
+
+static noinline void __init kmalloc_oob_memset_8(void)
+{
+	char *ptr;
+	size_t size = 8;
+
+	pr_info("out-of-bounds in memset8\n");
+	ptr = kmalloc(size, GFP_KERNEL);
+	if (!ptr) {
+		pr_err("Allocation failed\n");
+		return;
+	}
+
+	memset(ptr+1, 0, 8);
+	kfree(ptr);
+}
+
+static noinline void __init kmalloc_oob_memset_16(void)
+{
+	char *ptr;
+	size_t size = 16;
+
+	pr_info("out-of-bounds in memset16\n");
+	ptr = kmalloc(size, GFP_KERNEL);
+	if (!ptr) {
+		pr_err("Allocation failed\n");
+		return;
+	}
+
+	memset(ptr+1, 0, 16);
+	kfree(ptr);
+}
+
 static noinline void __init kmalloc_oob_in_memset(void)
 {
 	char *ptr;
@@ -264,6 +329,10 @@ static int __init kmalloc_tests_init(void)
 	kmalloc_oob_krealloc_less();
 	kmalloc_oob_16();
 	kmalloc_oob_in_memset();
+	kmalloc_oob_memset_2();
+	kmalloc_oob_memset_4();
+	kmalloc_oob_memset_8();
+	kmalloc_oob_memset_16();
 	kmalloc_uaf();
 	kmalloc_uaf_memset();
 	kmalloc_uaf2();
-- 
1.8.3.4

Re: [PATCH 1/2] lib: test_kasan: add some testcases

[Andrey Ryabinin]

On 09/09/2015 06:59 AM, Wang Long wrote:
> This patch add some out of bounds testcases to test_kasan
> module.
> 
> Signed-off-by: Wang Long <long.wanglong@huawei.com>

Acked-by: Andrey Ryabinin <aryabinin@virtuozzo.com>

[PATCH 2/2] kasan: Fix a type conversion error

[Wang Long]

The current KASAN code can find the following out-of-bounds
bugs:
	char *ptr;
	ptr = kmalloc(8, GFP_KERNEL);
	memset(ptr+7, 0, 2);

the cause of the problem is the type conversion error in
*memory_is_poisoned_n* function. So this patch fix that.

Signed-off-by: Wang Long <long.wanglong@huawei.com>
---
 mm/kasan/kasan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
index 7b28e9c..5d65d06 100644
--- a/mm/kasan/kasan.c
+++ b/mm/kasan/kasan.c
@@ -204,7 +204,7 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
 		s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
 
 		if (unlikely(ret != (unsigned long)last_shadow ||
-			((last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
+			((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
 			return true;
 	}
 	return false;
-- 
1.8.3.4

Re: [PATCH 2/2] kasan: Fix a type conversion error

[Vladimir Murzin]

On 09/09/15 04:59, Wang Long wrote:
> The current KASAN code can find the following out-of-bounds

Should it be "cannot"?

Vladimir

> bugs:
> 	char *ptr;
> 	ptr = kmalloc(8, GFP_KERNEL);
> 	memset(ptr+7, 0, 2);
> 
> the cause of the problem is the type conversion error in
> *memory_is_poisoned_n* function. So this patch fix that.
> 
> Signed-off-by: Wang Long <long.wanglong@huawei.com>
> ---
>  mm/kasan/kasan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 7b28e9c..5d65d06 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -204,7 +204,7 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
>  		s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
>  
>  		if (unlikely(ret != (unsigned long)last_shadow ||
> -			((last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
> +			((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
>  			return true;
>  	}
>  	return false;
> 

Re: [PATCH 2/2] kasan: Fix a type conversion error

[long.wanglong]

On 2015/9/9 17:01, Vladimir Murzin wrote:
> On 09/09/15 04:59, Wang Long wrote:
>> The current KASAN code can find the following out-of-bounds
> 
> Should it be "cannot"?
> 
> Vladimir
> 
sorry for that mistake, it should be "cannot".

>> bugs:
>> 	char *ptr;
>> 	ptr = kmalloc(8, GFP_KERNEL);
>> 	memset(ptr+7, 0, 2);
>>
>> the cause of the problem is the type conversion error in
>> *memory_is_poisoned_n* function. So this patch fix that.
>>
>> Signed-off-by: Wang Long <long.wanglong@huawei.com>
>> ---
>>  mm/kasan/kasan.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 7b28e9c..5d65d06 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -204,7 +204,7 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
>>  		s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
>>  
>>  		if (unlikely(ret != (unsigned long)last_shadow ||
>> -			((last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
>> +			((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
>>  			return true;
>>  	}
>>  	return false;
>>
> 
> 
> .
> 

Re: [PATCH 2/2] kasan: Fix a type conversion error

[Andrey Ryabinin]

2015-09-09 6:59 GMT+03:00 Wang Long <long.wanglong@huawei.com>:
> The current KASAN code can find the following out-of-bounds
> bugs:
>         char *ptr;
>         ptr = kmalloc(8, GFP_KERNEL);
>         memset(ptr+7, 0, 2);
>
> the cause of the problem is the type conversion error in
> *memory_is_poisoned_n* function. So this patch fix that.
>
> Signed-off-by: Wang Long <long.wanglong@huawei.com>
> ---
>  mm/kasan/kasan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 7b28e9c..5d65d06 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -204,7 +204,7 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
>                 s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
>
>                 if (unlikely(ret != (unsigned long)last_shadow ||
> -                       ((last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
> +                       ((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))

Is there any problem if we just define last_byte as 'long' instead of
'unsigned long' ?

>                         return true;
>         }
>         return false;
> --
> 1.8.3.4
>

Re: [PATCH 2/2] kasan: Fix a type conversion error

[long.wanglong]

On 2015/9/9 17:40, Andrey Ryabinin wrote:
> 2015-09-09 6:59 GMT+03:00 Wang Long <long.wanglong@huawei.com>:
>> The current KASAN code can find the following out-of-bounds
>> bugs:
>>         char *ptr;
>>         ptr = kmalloc(8, GFP_KERNEL);
>>         memset(ptr+7, 0, 2);
>>
>> the cause of the problem is the type conversion error in
>> *memory_is_poisoned_n* function. So this patch fix that.
>>
>> Signed-off-by: Wang Long <long.wanglong@huawei.com>
>> ---
>>  mm/kasan/kasan.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
>> index 7b28e9c..5d65d06 100644
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -204,7 +204,7 @@ static __always_inline bool memory_is_poisoned_n(unsigned long addr,
>>                 s8 *last_shadow = (s8 *)kasan_mem_to_shadow((void *)last_byte);
>>
>>                 if (unlikely(ret != (unsigned long)last_shadow ||
>> -                       ((last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
>> +                       ((long)(last_byte & KASAN_SHADOW_MASK) >= *last_shadow)))
> 
> Is there any problem if we just define last_byte as 'long' instead of
> 'unsigned long' ?

yes, I think it is not OK, because on my test, if we define last_byte as 'long'
instead of 'unsigned long', the bug we talk about can not be found.


> 
>>                         return true;
>>         }
>>         return false;
>> --
>> 1.8.3.4
>>
> 
> .
>