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
