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

[Mark Rutland]

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@xxxxxxxxxx>
> >>---
> >> 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.

> 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.