* Re: [PATCH] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-25 6:46 [PATCH] ARM: kasan: Fix __get_user_check failure with kasan Lexi Shao
@ 2021-08-25 9:06 ` Dmitry Osipenko
2021-08-28 3:26 ` Lexi Shao
2021-08-27 12:39 ` Kefeng Wang
2021-08-27 12:56 ` Russell King (Oracle)
2 siblings, 1 reply; 10+ messages in thread
From: Dmitry Osipenko @ 2021-08-25 9:06 UTC (permalink / raw)
To: Lexi Shao, linux-arm-kernel
Cc: linux, dvyukov, ryabinin.a.a, glider, andreyknvl, linus.walleij,
liuwenliang, nixiaoming, qiuxi1, wangkefeng.wang
25.08.2021 09:46, Lexi Shao пишет:
> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
> error code is store in register int __e(r0). When kasan is
> enabled, assigning value to kernel address might trigger kasan check,
> which unexpectedly overwrites r0 and causes undefined behavior on arm
> kasan images.
>
> One example is failure in do_futex and results in process soft lockup.
> Log:
> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
> Q:Reg:1151]
> ...
> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
> (futex_wait_setup) from (futex_wait+0x138/0x394)
> (futex_wait) from (do_futex+0x164/0xe40)
> (do_futex) from (sys_futex_time32+0x178/0x230)
> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>
> The soft lockup happens in function futex_wait_setup. The reason is
> function get_futex_value_locked always return EINVAL, thus pc jump
> back to retry label and causes looping.
>
> The assembly code of get_futex_value_locked in kernel/futex.c:
> ...
> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
> c01f6dcc: e1a00007 mov r0, r7
> c01f6dd0: e1a05002 mov r5, r2
> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
> c01f6dd8: e5875000 str r5, [r7]
> // save ret value of __get_user(*dest, from), which is dest address now
> c01f6ddc: e1a05000 mov r5, r0
> ...
> // checking return value of __get_user failed
> c01f6e00: e3550000 cmp r5, #0
> ...
> c01f6e0c: 01a00005 moveq r0, r5
> // assign return value to EINVAL
> c01f6e10: 13e0000d mvnne r0, #13
>
> Return value is the destination address of get_user thus certainly
> non-zero, so get_futex_value_locked always return EINVAL.
>
> Fix it by using a tmp vairable to store the error code before the
> assignment. This fix has no effects to non-kasan images thanks to compiler
> optimization. It only affects cases that overwrite r0 due to kasan check.
>
> This should fix bug discussed in link:
> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>
> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
> ---
> arch/arm/include/asm/uaccess.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index a13d90206472..a6eb9af74870 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
> register unsigned long __l asm("r1") = __limit; \
> register int __e asm("r0"); \
> unsigned int __ua_flags = uaccess_save_and_enable(); \
> + int __tmp_e; \
> switch (sizeof(*(__p))) { \
> case 1: \
> if (sizeof((x)) >= 8) \
> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
> break; \
> default: __e = __get_user_bad(); break; \
> } \
> + __tmp_e = __e; \
> uaccess_restore(__ua_flags); \
> x = (typeof(*(p))) __r2; \
> - __e; \
> + __e = __tmp_e; \
> })
>
> #define get_user(x, p) \
>
I successfully loaded KDE Plasma with this fix, which was impossible
previously. Thank you!
Is it guaranteed that r2 register won't be clobbered as well?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-25 9:06 ` Dmitry Osipenko
@ 2021-08-28 3:26 ` Lexi Shao
0 siblings, 0 replies; 10+ messages in thread
From: Lexi Shao @ 2021-08-28 3:26 UTC (permalink / raw)
To: digetx
Cc: andreyknvl, dvyukov, glider, linus.walleij, linux-arm-kernel,
linux, liuwenliang, nixiaoming, qiuxi1, ryabinin.a.a, shaolexi,
wangkefeng.wang
>25.08.2021 09:46, Lexi Shao пишет:
>> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
>> error code is store in register int __e(r0). When kasan is
>> enabled, assigning value to kernel address might trigger kasan check,
>> which unexpectedly overwrites r0 and causes undefined behavior on arm
>> kasan images.
>>
>> One example is failure in do_futex and results in process soft lockup.
>> Log:
>> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
>> Q:Reg:1151]
>> ...
>> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
>> (futex_wait_setup) from (futex_wait+0x138/0x394)
>> (futex_wait) from (do_futex+0x164/0xe40)
>> (do_futex) from (sys_futex_time32+0x178/0x230)
>> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>>
>> The soft lockup happens in function futex_wait_setup. The reason is
>> function get_futex_value_locked always return EINVAL, thus pc jump
>> back to retry label and causes looping.
>>
>> The assembly code of get_futex_value_locked in kernel/futex.c:
>> ...
>> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
>> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
>> c01f6dcc: e1a00007 mov r0, r7
>> c01f6dd0: e1a05002 mov r5, r2
>> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
>> c01f6dd8: e5875000 str r5, [r7]
>> // save ret value of __get_user(*dest, from), which is dest address now
>> c01f6ddc: e1a05000 mov r5, r0
>> ...
>> // checking return value of __get_user failed
>> c01f6e00: e3550000 cmp r5, #0
>> ...
>> c01f6e0c: 01a00005 moveq r0, r5
>> // assign return value to EINVAL
>> c01f6e10: 13e0000d mvnne r0, #13
>>
>> Return value is the destination address of get_user thus certainly
>> non-zero, so get_futex_value_locked always return EINVAL.
>>
>> Fix it by using a tmp vairable to store the error code before the
>> assignment. This fix has no effects to non-kasan images thanks to compiler
>> optimization. It only affects cases that overwrite r0 due to kasan check.
>>
>> This should fix bug discussed in link:
>> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>>
>> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
>> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
>> ---
>> arch/arm/include/asm/uaccess.h | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
>> index a13d90206472..a6eb9af74870 100644
>> --- a/arch/arm/include/asm/uaccess.h
>> +++ b/arch/arm/include/asm/uaccess.h
>> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
>> register unsigned long __l asm("r1") = __limit; \
>> register int __e asm("r0"); \
>> unsigned int __ua_flags = uaccess_save_and_enable(); \
>> + int __tmp_e; \
>> switch (sizeof(*(__p))) { \
>> case 1: \
>> if (sizeof((x)) >= 8) \
>> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
>> break; \
>> default: __e = __get_user_bad(); break; \
>> } \
>> + __tmp_e = __e; \
>> uaccess_restore(__ua_flags); \
>> x = (typeof(*(p))) __r2; \
>> - __e; \
>> + __e = __tmp_e; \
>> })
>>
>> #define get_user(x, p) \
>>
>
>I successfully loaded KDE Plasma with this fix, which was impossible
>previously. Thank you!
>
>Is it guaranteed that r2 register won't be clobbered as well?
Value of r2 is saved before calling __asan_store4:
c01f6dbc: e5951008 ldr r1, [r5, #8]
c01f6dc0: e1a00008 mov r0, r8
c01f6dc4: e2411001 sub r1, r1, #1
c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
c01f6dcc: e1a00007 mov r0, r7
// move value of r2 to r5
c01f6dd0: e1a05002 mov r5, r2
c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
// the assignment, saving r5(orig r2) to dest
c01f6dd8: e5875000 str r5, [r7]
c01f6ddc: e1a05000 mov r5, r0
I can't explain it clearly why the compiler save r2 but not r0 though...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-25 6:46 [PATCH] ARM: kasan: Fix __get_user_check failure with kasan Lexi Shao
2021-08-25 9:06 ` Dmitry Osipenko
@ 2021-08-27 12:39 ` Kefeng Wang
2021-08-27 12:56 ` Russell King (Oracle)
2 siblings, 0 replies; 10+ messages in thread
From: Kefeng Wang @ 2021-08-27 12:39 UTC (permalink / raw)
To: Lexi Shao, linux-arm-kernel
Cc: linux, dvyukov, ryabinin.a.a, glider, andreyknvl, digetx,
linus.walleij, liuwenliang, nixiaoming, qiuxi1
On 2021/8/25 14:46, Lexi Shao wrote:
> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
> error code is store in register int __e(r0). When kasan is
> enabled, assigning value to kernel address might trigger kasan check,
> which unexpectedly overwrites r0 and causes undefined behavior on arm
> kasan images.
>
> One example is failure in do_futex and results in process soft lockup.
> Log:
> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
> Q:Reg:1151]
> ...
> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
> (futex_wait_setup) from (futex_wait+0x138/0x394)
> (futex_wait) from (do_futex+0x164/0xe40)
> (do_futex) from (sys_futex_time32+0x178/0x230)
> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>
> The soft lockup happens in function futex_wait_setup. The reason is
> function get_futex_value_locked always return EINVAL, thus pc jump
> back to retry label and causes looping.
>
> The assembly code of get_futex_value_locked in kernel/futex.c:
> ...
> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
> c01f6dcc: e1a00007 mov r0, r7
> c01f6dd0: e1a05002 mov r5, r2
> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
> c01f6dd8: e5875000 str r5, [r7]
> // save ret value of __get_user(*dest, from), which is dest address now
> c01f6ddc: e1a05000 mov r5, r0
> ...
> // checking return value of __get_user failed
> c01f6e00: e3550000 cmp r5, #0
> ...
> c01f6e0c: 01a00005 moveq r0, r5
> // assign return value to EINVAL
> c01f6e10: 13e0000d mvnne r0, #13
>
> Return value is the destination address of get_user thus certainly
> non-zero, so get_futex_value_locked always return EINVAL.
>
> Fix it by using a tmp vairable to store the error code before the
> assignment. This fix has no effects to non-kasan images thanks to compiler
> optimization. It only affects cases that overwrite r0 due to kasan check.
>
> This should fix bug discussed in link:
> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>
> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
> ---
Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-25 6:46 [PATCH] ARM: kasan: Fix __get_user_check failure with kasan Lexi Shao
2021-08-25 9:06 ` Dmitry Osipenko
2021-08-27 12:39 ` Kefeng Wang
@ 2021-08-27 12:56 ` Russell King (Oracle)
2021-08-28 2:21 ` Lexi Shao
2021-08-28 2:25 ` [PATCH v2] " Lexi Shao
2 siblings, 2 replies; 10+ messages in thread
From: Russell King (Oracle) @ 2021-08-27 12:56 UTC (permalink / raw)
To: Lexi Shao
Cc: linux-arm-kernel, dvyukov, ryabinin.a.a, glider, andreyknvl,
digetx, linus.walleij, liuwenliang, nixiaoming, qiuxi1,
wangkefeng.wang
On Wed, Aug 25, 2021 at 02:46:50PM +0800, Lexi Shao wrote:
> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
> error code is store in register int __e(r0). When kasan is
> enabled, assigning value to kernel address might trigger kasan check,
> which unexpectedly overwrites r0 and causes undefined behavior on arm
> kasan images.
>
> One example is failure in do_futex and results in process soft lockup.
> Log:
> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
> Q:Reg:1151]
> ...
> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
> (futex_wait_setup) from (futex_wait+0x138/0x394)
> (futex_wait) from (do_futex+0x164/0xe40)
> (do_futex) from (sys_futex_time32+0x178/0x230)
> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>
> The soft lockup happens in function futex_wait_setup. The reason is
> function get_futex_value_locked always return EINVAL, thus pc jump
> back to retry label and causes looping.
>
> The assembly code of get_futex_value_locked in kernel/futex.c:
> ...
> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
> c01f6dcc: e1a00007 mov r0, r7
> c01f6dd0: e1a05002 mov r5, r2
> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
> c01f6dd8: e5875000 str r5, [r7]
> // save ret value of __get_user(*dest, from), which is dest address now
> c01f6ddc: e1a05000 mov r5, r0
> ...
> // checking return value of __get_user failed
> c01f6e00: e3550000 cmp r5, #0
> ...
> c01f6e0c: 01a00005 moveq r0, r5
> // assign return value to EINVAL
> c01f6e10: 13e0000d mvnne r0, #13
>
> Return value is the destination address of get_user thus certainly
> non-zero, so get_futex_value_locked always return EINVAL.
This description doesn't actually make it clear why this is failing in
this case, until one looks at get_futex_value_locked() and realises
that what is actually going on here is:
*dest = (typeof(*(p))) __r2;
which is why we end up with the store there.
> Fix it by using a tmp vairable to store the error code before the
> assignment. This fix has no effects to non-kasan images thanks to compiler
> optimization. It only affects cases that overwrite r0 due to kasan check.
>
> This should fix bug discussed in link:
> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>
> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
> ---
> arch/arm/include/asm/uaccess.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index a13d90206472..a6eb9af74870 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
> register unsigned long __l asm("r1") = __limit; \
> register int __e asm("r0"); \
> unsigned int __ua_flags = uaccess_save_and_enable(); \
> + int __tmp_e; \
> switch (sizeof(*(__p))) { \
> case 1: \
> if (sizeof((x)) >= 8) \
> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
> break; \
> default: __e = __get_user_bad(); break; \
> } \
> + __tmp_e = __e; \
> uaccess_restore(__ua_flags); \
> x = (typeof(*(p))) __r2; \
> - __e; \
> + __e = __tmp_e; \
There is no need to re-assign __tmp_e back to __e - you can just return
__tmp_e here.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-27 12:56 ` Russell King (Oracle)
@ 2021-08-28 2:21 ` Lexi Shao
2021-08-28 2:25 ` [PATCH v2] " Lexi Shao
1 sibling, 0 replies; 10+ messages in thread
From: Lexi Shao @ 2021-08-28 2:21 UTC (permalink / raw)
To: linux
Cc: andreyknvl, digetx, dvyukov, glider, linus.walleij,
linux-arm-kernel, liuwenliang, nixiaoming, qiuxi1, ryabinin.a.a,
shaolexi, wangkefeng.wang
>On Wed, Aug 25, 2021 at 02:46:50PM +0800, Lexi Shao wrote:
>> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
>> error code is store in register int __e(r0). When kasan is
>> enabled, assigning value to kernel address might trigger kasan check,
>> which unexpectedly overwrites r0 and causes undefined behavior on arm
>> kasan images.
>>
>> One example is failure in do_futex and results in process soft lockup.
>> Log:
>> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
>> Q:Reg:1151]
>> ...
>> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
>> (futex_wait_setup) from (futex_wait+0x138/0x394)
>> (futex_wait) from (do_futex+0x164/0xe40)
>> (do_futex) from (sys_futex_time32+0x178/0x230)
>> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>>
>> The soft lockup happens in function futex_wait_setup. The reason is
>> function get_futex_value_locked always return EINVAL, thus pc jump
>> back to retry label and causes looping.
>>
>> The assembly code of get_futex_value_locked in kernel/futex.c:
>> ...
>> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
>> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
>> c01f6dcc: e1a00007 mov r0, r7
>> c01f6dd0: e1a05002 mov r5, r2
>> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
>> c01f6dd8: e5875000 str r5, [r7]
>> // save ret value of __get_user(*dest, from), which is dest address now
>> c01f6ddc: e1a05000 mov r5, r0
>> ...
>> // checking return value of __get_user failed
>> c01f6e00: e3550000 cmp r5, #0
>> ...
>> c01f6e0c: 01a00005 moveq r0, r5
>> // assign return value to EINVAL
>> c01f6e10: 13e0000d mvnne r0, #13
>>
>> Return value is the destination address of get_user thus certainly
>> non-zero, so get_futex_value_locked always return EINVAL.
>
>This description doesn't actually make it clear why this is failing in
>this case, until one looks at get_futex_value_locked() and realises
>that what is actually going on here is:
>
> *dest = (typeof(*(p))) __r2;
>
>which is why we end up with the store there.
>
>> Fix it by using a tmp vairable to store the error code before the
>> assignment. This fix has no effects to non-kasan images thanks to compiler
>> optimization. It only affects cases that overwrite r0 due to kasan check.
>>
>> This should fix bug discussed in link:
>> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>>
>> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
>> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
>> ---
>> arch/arm/include/asm/uaccess.h | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
>> index a13d90206472..a6eb9af74870 100644
>> --- a/arch/arm/include/asm/uaccess.h
>> +++ b/arch/arm/include/asm/uaccess.h
>> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
>> register unsigned long __l asm("r1") = __limit; \
>> register int __e asm("r0"); \
>> unsigned int __ua_flags = uaccess_save_and_enable(); \
>> + int __tmp_e; \
>> switch (sizeof(*(__p))) { \
>> case 1: \
>> if (sizeof((x)) >= 8) \
>> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
>> break; \
>> default: __e = __get_user_bad(); break; \
>> } \
>> + __tmp_e = __e; \
>> uaccess_restore(__ua_flags); \
>> x = (typeof(*(p))) __r2; \
>> - __e; \
>> + __e = __tmp_e; \
>
>There is no need to re-assign __tmp_e back to __e - you can just return
>__tmp_e here.
Yes you are right, will send the updated patch in the next email.
Lexi
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-27 12:56 ` Russell King (Oracle)
2021-08-28 2:21 ` Lexi Shao
@ 2021-08-28 2:25 ` Lexi Shao
2021-08-31 9:51 ` Dmitry Osipenko
2021-09-01 7:05 ` Kefeng Wang
1 sibling, 2 replies; 10+ messages in thread
From: Lexi Shao @ 2021-08-28 2:25 UTC (permalink / raw)
To: linux-arm-kernel
Cc: linux, andreyknvl, digetx, dvyukov, glider, linus.walleij,
liuwenliang, nixiaoming, qiuxi1, ryabinin.a.a, shaolexi,
wangkefeng.wang
In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
error code is store in register int __e(r0). When kasan is
enabled, assigning value to kernel address might trigger kasan check,
which unexpectedly overwrites r0 and causes undefined behavior on arm
kasan images.
One example is failure in do_futex and results in process soft lockup.
Log:
watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
Q:Reg:1151]
...
(__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
(futex_wait_setup) from (futex_wait+0x138/0x394)
(futex_wait) from (do_futex+0x164/0xe40)
(do_futex) from (sys_futex_time32+0x178/0x230)
(sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
The soft lockup happens in function futex_wait_setup. The reason is
function get_futex_value_locked always return EINVAL, thus pc jump
back to retry label and causes looping.
This line in function get_futex_value_locked
ret = __get_user(*dest, from);
is expanded to
*dest = (typeof(*(p))) __r2; ,
in macro __get_user_check. Writing to pointer dest triggers kasan check
and overwrites the return value of __get_user_x function.
The assembly code of get_futex_value_locked in kernel/futex.c:
...
c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
// "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
c01f6dcc: e1a00007 mov r0, r7
c01f6dd0: e1a05002 mov r5, r2
c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
c01f6dd8: e5875000 str r5, [r7]
// save ret value of __get_user(*dest, from), which is dest address now
c01f6ddc: e1a05000 mov r5, r0
...
// checking return value of __get_user failed
c01f6e00: e3550000 cmp r5, #0
...
c01f6e0c: 01a00005 moveq r0, r5
// assign return value to EINVAL
c01f6e10: 13e0000d mvnne r0, #13
Return value is the destination address of get_user thus certainly
non-zero, so get_futex_value_locked always return EINVAL.
Fix it by using a tmp vairable to store the error code before the
assignment. This fix has no effects to non-kasan images thanks to compiler
optimization. It only affects cases that overwrite r0 due to kasan check.
This should fix bug discussed in link:
[1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
Signed-off-by: Lexi Shao <shaolexi@huawei.com>
---
arch/arm/include/asm/uaccess.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index a13d90206472..d9db752c51fe 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
register unsigned long __l asm("r1") = __limit; \
register int __e asm("r0"); \
unsigned int __ua_flags = uaccess_save_and_enable(); \
+ int __tmp_e; \
switch (sizeof(*(__p))) { \
case 1: \
if (sizeof((x)) >= 8) \
@@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
break; \
default: __e = __get_user_bad(); break; \
} \
+ __tmp_e = __e; \
uaccess_restore(__ua_flags); \
x = (typeof(*(p))) __r2; \
- __e; \
+ __tmp_e; \
})
#define get_user(x, p) \
--
2.12.3
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-28 2:25 ` [PATCH v2] " Lexi Shao
@ 2021-08-31 9:51 ` Dmitry Osipenko
2021-09-20 17:07 ` Dmitry Osipenko
2021-09-01 7:05 ` Kefeng Wang
1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Osipenko @ 2021-08-31 9:51 UTC (permalink / raw)
To: Lexi Shao, linux-arm-kernel
Cc: linux, andreyknvl, dvyukov, glider, linus.walleij, liuwenliang,
nixiaoming, qiuxi1, ryabinin.a.a, wangkefeng.wang
28.08.2021 05:25, Lexi Shao пишет:
> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
> error code is store in register int __e(r0). When kasan is
> enabled, assigning value to kernel address might trigger kasan check,
> which unexpectedly overwrites r0 and causes undefined behavior on arm
> kasan images.
>
> One example is failure in do_futex and results in process soft lockup.
> Log:
> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
> Q:Reg:1151]
> ...
> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
> (futex_wait_setup) from (futex_wait+0x138/0x394)
> (futex_wait) from (do_futex+0x164/0xe40)
> (do_futex) from (sys_futex_time32+0x178/0x230)
> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>
> The soft lockup happens in function futex_wait_setup. The reason is
> function get_futex_value_locked always return EINVAL, thus pc jump
> back to retry label and causes looping.
>
> This line in function get_futex_value_locked
> ret = __get_user(*dest, from);
> is expanded to
> *dest = (typeof(*(p))) __r2; ,
> in macro __get_user_check. Writing to pointer dest triggers kasan check
> and overwrites the return value of __get_user_x function.
> The assembly code of get_futex_value_locked in kernel/futex.c:
> ...
> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
> c01f6dcc: e1a00007 mov r0, r7
> c01f6dd0: e1a05002 mov r5, r2
> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
> c01f6dd8: e5875000 str r5, [r7]
> // save ret value of __get_user(*dest, from), which is dest address now
> c01f6ddc: e1a05000 mov r5, r0
> ...
> // checking return value of __get_user failed
> c01f6e00: e3550000 cmp r5, #0
> ...
> c01f6e0c: 01a00005 moveq r0, r5
> // assign return value to EINVAL
> c01f6e10: 13e0000d mvnne r0, #13
>
> Return value is the destination address of get_user thus certainly
> non-zero, so get_futex_value_locked always return EINVAL.
>
> Fix it by using a tmp vairable to store the error code before the
> assignment. This fix has no effects to non-kasan images thanks to compiler
> optimization. It only affects cases that overwrite r0 due to kasan check.
>
> This should fix bug discussed in link:
> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>
> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
> ---
> arch/arm/include/asm/uaccess.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index a13d90206472..d9db752c51fe 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
> register unsigned long __l asm("r1") = __limit; \
> register int __e asm("r0"); \
> unsigned int __ua_flags = uaccess_save_and_enable(); \
> + int __tmp_e; \
> switch (sizeof(*(__p))) { \
> case 1: \
> if (sizeof((x)) >= 8) \
> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
> break; \
> default: __e = __get_user_bad(); break; \
> } \
> + __tmp_e = __e; \
> uaccess_restore(__ua_flags); \
> x = (typeof(*(p))) __r2; \
> - __e; \
> + __tmp_e; \
> })
>
> #define get_user(x, p) \
>
Tested-by: Dmitry Osipenko <digetx@gmail.com> # Tegra
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-31 9:51 ` Dmitry Osipenko
@ 2021-09-20 17:07 ` Dmitry Osipenko
0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Osipenko @ 2021-09-20 17:07 UTC (permalink / raw)
To: Lexi Shao, linux-arm-kernel
Cc: linux, andreyknvl, dvyukov, glider, linus.walleij, liuwenliang,
nixiaoming, qiuxi1, ryabinin.a.a, wangkefeng.wang
31.08.2021 12:51, Dmitry Osipenko пишет:
> 28.08.2021 05:25, Lexi Shao пишет:
>> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
>> error code is store in register int __e(r0). When kasan is
>> enabled, assigning value to kernel address might trigger kasan check,
>> which unexpectedly overwrites r0 and causes undefined behavior on arm
>> kasan images.
>>
>> One example is failure in do_futex and results in process soft lockup.
>> Log:
>> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
>> Q:Reg:1151]
>> ...
>> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
>> (futex_wait_setup) from (futex_wait+0x138/0x394)
>> (futex_wait) from (do_futex+0x164/0xe40)
>> (do_futex) from (sys_futex_time32+0x178/0x230)
>> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>>
>> The soft lockup happens in function futex_wait_setup. The reason is
>> function get_futex_value_locked always return EINVAL, thus pc jump
>> back to retry label and causes looping.
>>
>> This line in function get_futex_value_locked
>> ret = __get_user(*dest, from);
>> is expanded to
>> *dest = (typeof(*(p))) __r2; ,
>> in macro __get_user_check. Writing to pointer dest triggers kasan check
>> and overwrites the return value of __get_user_x function.
>> The assembly code of get_futex_value_locked in kernel/futex.c:
>> ...
>> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
>> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
>> c01f6dcc: e1a00007 mov r0, r7
>> c01f6dd0: e1a05002 mov r5, r2
>> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
>> c01f6dd8: e5875000 str r5, [r7]
>> // save ret value of __get_user(*dest, from), which is dest address now
>> c01f6ddc: e1a05000 mov r5, r0
>> ...
>> // checking return value of __get_user failed
>> c01f6e00: e3550000 cmp r5, #0
>> ...
>> c01f6e0c: 01a00005 moveq r0, r5
>> // assign return value to EINVAL
>> c01f6e10: 13e0000d mvnne r0, #13
>>
>> Return value is the destination address of get_user thus certainly
>> non-zero, so get_futex_value_locked always return EINVAL.
>>
>> Fix it by using a tmp vairable to store the error code before the
>> assignment. This fix has no effects to non-kasan images thanks to compiler
>> optimization. It only affects cases that overwrite r0 due to kasan check.
>>
>> This should fix bug discussed in link:
>> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>>
>> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
>> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
>> ---
>> arch/arm/include/asm/uaccess.h | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
>> index a13d90206472..d9db752c51fe 100644
>> --- a/arch/arm/include/asm/uaccess.h
>> +++ b/arch/arm/include/asm/uaccess.h
>> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
>> register unsigned long __l asm("r1") = __limit; \
>> register int __e asm("r0"); \
>> unsigned int __ua_flags = uaccess_save_and_enable(); \
>> + int __tmp_e; \
>> switch (sizeof(*(__p))) { \
>> case 1: \
>> if (sizeof((x)) >= 8) \
>> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
>> break; \
>> default: __e = __get_user_bad(); break; \
>> } \
>> + __tmp_e = __e; \
>> uaccess_restore(__ua_flags); \
>> x = (typeof(*(p))) __r2; \
>> - __e; \
>> + __tmp_e; \
>> })
>>
>> #define get_user(x, p) \
>>
>
> Tested-by: Dmitry Osipenko <digetx@gmail.com> # Tegra
>
Note that you may need to submit this patch to
https://www.armlinux.org.uk/developer/patches/ to speed up the applying.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] ARM: kasan: Fix __get_user_check failure with kasan
2021-08-28 2:25 ` [PATCH v2] " Lexi Shao
2021-08-31 9:51 ` Dmitry Osipenko
@ 2021-09-01 7:05 ` Kefeng Wang
1 sibling, 0 replies; 10+ messages in thread
From: Kefeng Wang @ 2021-09-01 7:05 UTC (permalink / raw)
To: Lexi Shao, linux-arm-kernel
Cc: linux, andreyknvl, digetx, dvyukov, glider, linus.walleij,
liuwenliang, nixiaoming, qiuxi1, ryabinin.a.a
On 2021/8/28 10:25, Lexi Shao wrote:
> In macro __get_user_check defined in arch/arm/include/asm/uaccess.h,
> error code is store in register int __e(r0). When kasan is
> enabled, assigning value to kernel address might trigger kasan check,
> which unexpectedly overwrites r0 and causes undefined behavior on arm
> kasan images.
>
> One example is failure in do_futex and results in process soft lockup.
> Log:
> watchdog: BUG: soft lockup - CPU#0 stuck for 62946ms! [rs:main
> Q:Reg:1151]
> ...
> (__asan_store4) from (futex_wait_setup+0xf8/0x2b4)
> (futex_wait_setup) from (futex_wait+0x138/0x394)
> (futex_wait) from (do_futex+0x164/0xe40)
> (do_futex) from (sys_futex_time32+0x178/0x230)
> (sys_futex_time32) from (ret_fast_syscall+0x0/0x50)
>
> The soft lockup happens in function futex_wait_setup. The reason is
> function get_futex_value_locked always return EINVAL, thus pc jump
> back to retry label and causes looping.
>
> This line in function get_futex_value_locked
> ret = __get_user(*dest, from);
> is expanded to
> *dest = (typeof(*(p))) __r2; ,
> in macro __get_user_check. Writing to pointer dest triggers kasan check
> and overwrites the return value of __get_user_x function.
> The assembly code of get_futex_value_locked in kernel/futex.c:
> ...
> c01f6dc8: eb0b020e bl c04b7608 <__get_user_4>
> // "x = (typeof(*(p))) __r2;" triggers kasan check and r0 is overwritten
> c01f6dcc: e1a00007 mov r0, r7
> c01f6dd0: e1a05002 mov r5, r2
> c01f6dd4: eb04f1e6 bl c0333574 <__asan_store4>
> c01f6dd8: e5875000 str r5, [r7]
> // save ret value of __get_user(*dest, from), which is dest address now
> c01f6ddc: e1a05000 mov r5, r0
> ...
> // checking return value of __get_user failed
> c01f6e00: e3550000 cmp r5, #0
> ...
> c01f6e0c: 01a00005 moveq r0, r5
> // assign return value to EINVAL
> c01f6e10: 13e0000d mvnne r0, #13
>
> Return value is the destination address of get_user thus certainly
> non-zero, so get_futex_value_locked always return EINVAL.
>
> Fix it by using a tmp vairable to store the error code before the
> assignment. This fix has no effects to non-kasan images thanks to compiler
> optimization. It only affects cases that overwrite r0 due to kasan check.
>
> This should fix bug discussed in link:
> [1] https://lore.kernel.org/linux-arm-kernel/0ef7c2a5-5d8b-c5e0-63fa-31693fd4495c@gmail.com/
>
> Fixes: 421015713b30 ("ARM: 9017/2: Enable KASan for ARM")
> Signed-off-by: Lexi Shao <shaolexi@huawei.com>
Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> arch/arm/include/asm/uaccess.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index a13d90206472..d9db752c51fe 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -200,6 +200,7 @@ extern int __get_user_64t_4(void *);
> register unsigned long __l asm("r1") = __limit; \
> register int __e asm("r0"); \
> unsigned int __ua_flags = uaccess_save_and_enable(); \
> + int __tmp_e; \
> switch (sizeof(*(__p))) { \
> case 1: \
> if (sizeof((x)) >= 8) \
> @@ -227,9 +228,10 @@ extern int __get_user_64t_4(void *);
> break; \
> default: __e = __get_user_bad(); break; \
> } \
> + __tmp_e = __e; \
> uaccess_restore(__ua_flags); \
> x = (typeof(*(p))) __r2; \
> - __e; \
> + __tmp_e; \
> })
>
> #define get_user(x, p) \
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 10+ messages in thread