Re: [PATCH] arm64: kasan: instrument user memory access API

["Shi, Yang" <yang.shi@linaro.org>]

On 5/27/2016 10:46 AM, Mark Rutland wrote:
> On Fri, May 27, 2016 at 09:34:03AM -0700, Shi, Yang wrote:
>> On 5/27/2016 4:02 AM, Andrey Ryabinin wrote:
>>>
>>>
>>> On 05/26/2016 09:43 PM, Yang Shi wrote:
>>>> The upstream commit 1771c6e1a567ea0ba2cccc0a4ffe68a1419fd8ef
>>>> ("x86/kasan: instrument user memory access API") added KASAN instrument to
>>>> x86 user memory access API, so added such instrument to ARM64 too.
>>>>
>>>> Tested by test_kasan module.
>>>>
>>>> Signed-off-by: Yang Shi <yang.shi@linaro.org>
>>>> ---
>>>> arch/arm64/include/asm/uaccess.h | 18 ++++++++++++++++--
>>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> Please, cover __copy_from_user() and __copy_to_user() too.
>>> Unlike x86, your patch doesn't instrument these two.
>
> Argh, I missed those when reviewing. My bad.
>
>> I should elaborated this in my review. Yes, I did think about it,
>> but unlike x86, __copy_to/from_user are implemented by asm code on
>> ARM64. If I add kasan_check_read/write into them, I have to move the
>> registers around to prepare the parameters for kasan calls, then
>> restore them after the call, for example the below code for
>> __copy_to_user:
>>
>>         mov     x9, x0
>>         mov     x10, x1
>>         mov     x11, x2
>>         mov     x0, x10
>>         mov     x1, x11
>>         bl      kasan_check_read
>>         mov     x0, x9
>>         mov     x1, x10
>
> There's no need to alter the assembly.
>
> Rename the functions (e.g. have __arch_raw_copy_from_user), and add
> static inline wrappers in uaccess.h that do the kasan calls before
> calling the assembly functions.
>
> That gives the compiler the freedom to do the right thing, and avoids
> horrible ifdeffery in the assembly code.

Thanks for the suggestion, will address in v2.

Yang

>
>> So, I'm wondering if it is worth or not since __copy_to/from_user
>> are just called at a couple of places, i.e. sctp, a couple of
>> drivers, etc and not used too much.
>
> [mark@leverpostej:~/src/linux]% git grep -w __copy_to_user -- ^arch | wc -l
> 63
> [mark@leverpostej:~/src/linux]% git grep -w __copy_from_user -- ^arch | wc -l
> 47
>
> That's a reasonable number of callsites.
>
> If we're going to bother adding this, it should be complete. So please
> do update __copy_from_user and __copy_to_user.
>
>> Actually, I think some of them
>> could be replaced by __copy_to/from_user_inatomic.
>
> Given the number of existing callers outside of arch code, I think we'll
> get far more traction reworking the arm64 parts for now.
>
> Thanks,
> Mark.
>