Re: Re: [PATCH v2] arm64: kasan: add interceptors for strcmp/strncmp functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Andrey,

Thanks for your review.

On 2018-09-03 오후 6:40, Andrey Ryabinin wrote:


On 08/23/2018 11:56 AM, Kyeongdon Kim wrote:

> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index c3bd520..61ad7f1 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -304,6 +304,29 @@ void *memcpy(void *dest, const void *src, size_t len)
>
> return __memcpy(dest, src, len);
> }
> +#ifdef CONFIG_ARM64
> +/*
> + * Arch arm64 use assembly variant for strcmp/strncmp,
> + * xtensa use inline asm operations and x86_64 use c one,
> + * so now this interceptors only for arm64 kasan.
> + */
> +#undef strcmp
> +int strcmp(const char *cs, const char *ct)
> +{
> + check_memory_region((unsigned long)cs, 1, false, _RET_IP_);
> + check_memory_region((unsigned long)ct, 1, false, _RET_IP_);
> +

Well this is definitely wrong. strcmp() often accesses far more than one byte.

> + return __strcmp(cs, ct);
> +}
> +#undef strncmp
> +int strncmp(const char *cs, const char *ct, size_t len)
> +{
> + check_memory_region((unsigned long)cs, len, false, _RET_IP_);
> + check_memory_region((unsigned long)ct, len, false, _RET_IP_);

This will cause false positives. Both 'cs', and 'ct' could be less than len bytes.

There is no need in these interceptors, just use the C implementations from lib/string.c
like you did in your first patch.
The only thing that was wrong in the first patch is that assembly implementations
were compiled out instead of being declared week.

Well, at first I thought so..
I would remove diff code in /mm/kasan/kasan.c then use C implementations in lib/string.c
w/ assem implementations as weak :

diff --git a/lib/string.c b/lib/string.c
index 2c0900a..a18b18f 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -312,7 +312,7 @@ size_t strlcat(char *dest, const char *src, size_t count)
 EXPORT_SYMBOL(strlcat);
 #endif

-#ifndef __HAVE_ARCH_STRCMP
+#if (defined(CONFIG_ARM64) && defined(CONFIG_KASAN)) || !defined(__HAVE_ARCH_STRCMP)
 /**
  * strcmp - Compare two strings
  * @cs: One string
@@ -336,7 +336,7 @@ int strcmp(const char *cs, const char *ct)
 EXPORT_SYMBOL(strcmp);
 #endif

-#ifndef __HAVE_ARCH_STRNCMP
+#if (defined(CONFIG_ARM64) && defined(CONFIG_KASAN)) || !defined(__HAVE_ARCH_STRNCMP)
 /**
  * strncmp - Compare two length-limited strings

Can I get your opinion wrt this ?

Thanks,




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux