linux-kernel - Re: [PATCH 1/2] riscv: Omit optimized string routines when using KASAN

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8694fc12-dcd0-45d6-acf7-ab44b9e310bc@sifive.com>
Date: Wed, 14 Aug 2024 04:06:21 -0500
From: Samuel Holland <samuel.holland@...ive.com>
To: Alexandre Ghiti <alex@...ti.fr>, Palmer Dabbelt <palmer@...belt.com>,
 linux-riscv@...ts.infradead.org
Cc: Yury Norov <yury.norov@...il.com>,
 Rasmus Villemoes <linux@...musvillemoes.dk>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] riscv: Omit optimized string routines when using
 KASAN

Hi Alex,

On 2024-08-01 5:26 AM, Alexandre Ghiti wrote:
> Hi Samuel,
> 
> On 01/08/2024 05:36, Samuel Holland wrote:
>> The optimized string routines are implemented in assembly, so they are
>> not instrumented for use with KASAN. Fall back to the C version of the
>> routines in order to improve KASAN coverage. This fixes the
>> kasan_strings() unit test.
>>
>> Signed-off-by: Samuel Holland <samuel.holland@...ive.com>
>> ---
>>
>>   arch/riscv/include/asm/string.h | 2 ++
>>   arch/riscv/kernel/riscv_ksyms.c | 3 ---
>>   arch/riscv/lib/Makefile         | 2 ++
>>   arch/riscv/lib/strcmp.S         | 1 +
>>   arch/riscv/lib/strlen.S         | 1 +
>>   arch/riscv/lib/strncmp.S        | 1 +
>>   arch/riscv/purgatory/Makefile   | 2 ++
>>   7 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/string.h b/arch/riscv/include/asm/string.h
>> index a96b1fea24fe..5ba77f60bf0b 100644
>> --- a/arch/riscv/include/asm/string.h
>> +++ b/arch/riscv/include/asm/string.h
>> @@ -19,6 +19,7 @@ extern asmlinkage void *__memcpy(void *, const void *, size_t);
>>   extern asmlinkage void *memmove(void *, const void *, size_t);
>>   extern asmlinkage void *__memmove(void *, const void *, size_t);
>>   +#if !(defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS))
> 
> 
> We do not support KASAN_SW_TAGS so there is no need for this #ifdef.

I just sent an RFC implementation of KASAN_SW_TAGS[1] (which you wouldn't have
known I was working on at the time :) ). Since these changes will be needed for
both modes, it made sense to me to go ahead and cover both at once.

Regards,
Samuel

[1]: https://lore.kernel.org/all/20240814085618.968833-1-samuel.holland@sifive.com/

>>   #define __HAVE_ARCH_STRCMP
>>   extern asmlinkage int strcmp(const char *cs, const char *ct);
>>   @@ -27,6 +28,7 @@ extern asmlinkage __kernel_size_t strlen(const char *);
>>     #define __HAVE_ARCH_STRNCMP
>>   extern asmlinkage int strncmp(const char *cs, const char *ct, size_t count);
>> +#endif
>>     /* For those files which don't want to check by kasan. */
>>   #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
>> diff --git a/arch/riscv/kernel/riscv_ksyms.c b/arch/riscv/kernel/riscv_ksyms.c
>> index a72879b4249a..5ab1c7e1a6ed 100644
>> --- a/arch/riscv/kernel/riscv_ksyms.c
>> +++ b/arch/riscv/kernel/riscv_ksyms.c
>> @@ -12,9 +12,6 @@
>>   EXPORT_SYMBOL(memset);
>>   EXPORT_SYMBOL(memcpy);
>>   EXPORT_SYMBOL(memmove);
>> -EXPORT_SYMBOL(strcmp);
>> -EXPORT_SYMBOL(strlen);
>> -EXPORT_SYMBOL(strncmp);
>>   EXPORT_SYMBOL(__memset);
>>   EXPORT_SYMBOL(__memcpy);
>>   EXPORT_SYMBOL(__memmove);
>> diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
>> index 2b369f51b0a5..8eec6b69a875 100644
>> --- a/arch/riscv/lib/Makefile
>> +++ b/arch/riscv/lib/Makefile
>> @@ -3,9 +3,11 @@ lib-y            += delay.o
>>   lib-y            += memcpy.o
>>   lib-y            += memset.o
>>   lib-y            += memmove.o
>> +ifeq ($(CONFIG_KASAN_GENERIC)$(CONFIG_KASAN_SW_TAGS),)
>>   lib-y            += strcmp.o
>>   lib-y            += strlen.o
>>   lib-y            += strncmp.o
>> +endif
>>   lib-y            += csum.o
>>   ifeq ($(CONFIG_MMU), y)
>>   lib-$(CONFIG_RISCV_ISA_V)    += uaccess_vector.o
>> diff --git a/arch/riscv/lib/strcmp.S b/arch/riscv/lib/strcmp.S
>> index 687b2bea5c43..542301a67a2f 100644
>> --- a/arch/riscv/lib/strcmp.S
>> +++ b/arch/riscv/lib/strcmp.S
>> @@ -120,3 +120,4 @@ strcmp_zbb:
>>   .option pop
>>   #endif
>>   SYM_FUNC_END(strcmp)
>> +EXPORT_SYMBOL(strcmp)
>> diff --git a/arch/riscv/lib/strlen.S b/arch/riscv/lib/strlen.S
>> index 8ae3064e45ff..962983b73251 100644
>> --- a/arch/riscv/lib/strlen.S
>> +++ b/arch/riscv/lib/strlen.S
>> @@ -131,3 +131,4 @@ strlen_zbb:
>>   #endif
>>   SYM_FUNC_END(strlen)
>>   SYM_FUNC_ALIAS(__pi_strlen, strlen)
>> +EXPORT_SYMBOL(strlen)
>> diff --git a/arch/riscv/lib/strncmp.S b/arch/riscv/lib/strncmp.S
>> index aba5b3148621..0f359ea2f55b 100644
>> --- a/arch/riscv/lib/strncmp.S
>> +++ b/arch/riscv/lib/strncmp.S
>> @@ -136,3 +136,4 @@ strncmp_zbb:
>>   .option pop
>>   #endif
>>   SYM_FUNC_END(strncmp)
>> +EXPORT_SYMBOL(strncmp)
>> diff --git a/arch/riscv/purgatory/Makefile b/arch/riscv/purgatory/Makefile
>> index f11945ee2490..fb9c917c9b45 100644
>> --- a/arch/riscv/purgatory/Makefile
>> +++ b/arch/riscv/purgatory/Makefile
>> @@ -1,7 +1,9 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>     purgatory-y := purgatory.o sha256.o entry.o string.o ctype.o memcpy.o
>> memset.o
>> +ifeq ($(CONFIG_KASAN_GENERIC)$(CONFIG_KASAN_SW_TAGS),)
>>   purgatory-y += strcmp.o strlen.o strncmp.o
>> +endif
>>     targets += $(purgatory-y)
>>   PURGATORY_OBJS = $(addprefix $(obj)/,$(purgatory-y))
> 
> 
> With the removal of KASAN_SW_TAGS, you can add:
> 
> Reviewed-by: Alexandre Ghiti <alexghiti@...osinc.com>
> 
> And since I have this testsuite in my CI, I gave it a try and it works so:
> 
> Tested-by: Alexandre Ghiti <alexghiti@...osinc.com>
> 
> Thanks,
> 
> Alex
> 
> 
>