Re: [RFC PATCH v2 3/3] kasan: add interceptors for all string functions

From: Christophe Leroy
Date: Tue Apr 02 2019 - 16:36:49 EST




Le 02/04/2019 Ã 18:14, Andrey Ryabinin a ÃcritÂ:


On 4/2/19 12:43 PM, Christophe Leroy wrote:
Hi Dmitry, Andrey and others,

Do you have any comments to this series ?


I don't see justification for adding all these non-instrumented functions. We need only some subset of these functions
and only on powerpc so far. Arches that don't use str*() that early simply doesn't need not-instrumented __str*() variant.

Also I don't think that auto-replace str* to __str* for all not instrumented files is a good idea, as this will reduce KASAN coverage.
E.g. we don't instrument slub.c but there is no reason to use non-instrumented __str*() functions there.

Ok, I didn't see it that way.

In fact I was seeing the opposite and was considering it as an opportunity to increase KASAN coverage. E.g.: at the time being things like the above (from arch/xtensa/include/asm/string.h) are not covered at all I believe:

#define __HAVE_ARCH_STRCPY
static inline char *strcpy(char *__dest, const char *__src)
{
register char *__xdest = __dest;
unsigned long __dummy;

__asm__ __volatile__("1:\n\t"
"l8ui %2, %1, 0\n\t"
"s8i %2, %0, 0\n\t"
"addi %1, %1, 1\n\t"
"addi %0, %0, 1\n\t"
"bnez %2, 1b\n\t"
: "=r" (__dest), "=r" (__src), "=&r" (__dummy)
: "0" (__dest), "1" (__src)
: "memory");

return __xdest;
}

In my series, I have deactivated optimised string functions when KASAN is selected like arm64 do. See https://patchwork.ozlabs.org/patch/1055780/
But not every arch does that, meaning that some string functions remains not instrumented at all.

Also, I was seeing it as a way to reduce impact on performance with KASAN. Because instrumenting each byte access of the non-optimised string functions is a performance genocide.


And finally, this series make bug reporting slightly worse. E.g. let's look at strcpy():

+char *strcpy(char *dest, const char *src)
+{
+ size_t len = __strlen(src) + 1;
+
+ check_memory_region((unsigned long)src, len, false, _RET_IP_);
+ check_memory_region((unsigned long)dest, len, true, _RET_IP_);
+
+ return __strcpy(dest, src);
+}

If src is not-null terminated string we might not see proper out-of-bounds report from KASAN only a crash in __strlen().
Which might make harder to identify where 'src' comes from, where it was allocated and what's the size of allocated area.


I'd like to know if this approach is ok or if it is better to keep doing as in https://patchwork.ozlabs.org/patch/1055788/

I think the patch from link is a better solution to the problem.


Ok, I'll stick with it then. Thanks for your feedback

Christophe