* [PATCH] powerpc: Fix __clear_user() with KUAP enabled
@ 2019-12-09 10:59 Andrew Donnellan
2019-12-09 11:50 ` Christophe Leroy
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Donnellan @ 2019-12-09 10:59 UTC (permalink / raw)
To: linuxppc-dev; +Cc: syzbot+f25ecf4b2982d8c7a640, Daniel Axtens
The KUAP implementation adds calls in clear_user() to enable and disable
access to userspace memory. However, it doesn't add these to
__clear_user(), which is used in the ptrace regset code.
As there's only one direct user of __clear_user(), and the time taken to
set the AMR for KUAP purposes is going to dominate the cost of a quick
access_ok(), there's not much point having a separate path.
Rename __clear_user() to clear_user_asm(), and make __clear_user() just
call clear_user().
Reported-by: syzbot+f25ecf4b2982d8c7a640@syzkaller-ppc64.appspotmail.com
Reported-by: Daniel Axtens <dja@axtens.net>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Russell Currey <ruscur@russell.cc>
Fixes: de78a9c42a79 ("powerpc: Add a framework for Kernel Userspace Access Protection")
Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
---
arch/powerpc/include/asm/uaccess.h | 9 +++++++--
arch/powerpc/lib/string_32.S | 4 ++--
arch/powerpc/lib/string_64.S | 6 +++---
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 15002b51ff18..d05bc0a4cafa 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -401,7 +401,7 @@ copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
return n;
}
-extern unsigned long __clear_user(void __user *addr, unsigned long size);
+extern unsigned long clear_user_asm(void __user *addr, unsigned long size);
static inline unsigned long clear_user(void __user *addr, unsigned long size)
{
@@ -409,12 +409,17 @@ static inline unsigned long clear_user(void __user *addr, unsigned long size)
might_fault();
if (likely(access_ok(addr, size))) {
allow_write_to_user(addr, size);
- ret = __clear_user(addr, size);
+ ret = clear_user_asm(addr, size);
prevent_write_to_user(addr, size);
}
return ret;
}
+static inline unsigned long __clear_user(void __user *addr, unsigned long size)
+{
+ return clear_user(addr, size);
+}
+
extern long strncpy_from_user(char *dst, const char __user *src, long count);
extern __must_check long strnlen_user(const char __user *str, long n);
diff --git a/arch/powerpc/lib/string_32.S b/arch/powerpc/lib/string_32.S
index f69a6aab7bfb..39847c630470 100644
--- a/arch/powerpc/lib/string_32.S
+++ b/arch/powerpc/lib/string_32.S
@@ -17,7 +17,7 @@ CACHELINE_BYTES = L1_CACHE_BYTES
LG_CACHELINE_BYTES = L1_CACHE_SHIFT
CACHELINE_MASK = (L1_CACHE_BYTES-1)
-_GLOBAL(__clear_user)
+_GLOBAL(clear_user_asm)
/*
* Use dcbz on the complete cache lines in the destination
* to set them to zero. This requires that the destination
@@ -87,4 +87,4 @@ _GLOBAL(__clear_user)
EX_TABLE(8b, 91b)
EX_TABLE(9b, 91b)
-EXPORT_SYMBOL(__clear_user)
+EXPORT_SYMBOL(clear_user_asm)
diff --git a/arch/powerpc/lib/string_64.S b/arch/powerpc/lib/string_64.S
index 507b18b1660e..a955f5e72feb 100644
--- a/arch/powerpc/lib/string_64.S
+++ b/arch/powerpc/lib/string_64.S
@@ -17,7 +17,7 @@ PPC64_CACHES:
.section ".text"
/**
- * __clear_user: - Zero a block of memory in user space, with less checking.
+ * clear_user_asm: - Zero a block of memory in user space, with less checking.
* @to: Destination address, in user space.
* @n: Number of bytes to zero.
*
@@ -58,7 +58,7 @@ err3; stb r0,0(r3)
mr r3,r4
blr
-_GLOBAL_TOC(__clear_user)
+_GLOBAL_TOC(clear_user_asm)
cmpdi r4,32
neg r6,r3
li r0,0
@@ -181,4 +181,4 @@ err1; dcbz 0,r3
cmpdi r4,32
blt .Lshort_clear
b .Lmedium_clear
-EXPORT_SYMBOL(__clear_user)
+EXPORT_SYMBOL(clear_user_asm)
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix __clear_user() with KUAP enabled
2019-12-09 10:59 [PATCH] powerpc: Fix __clear_user() with KUAP enabled Andrew Donnellan
@ 2019-12-09 11:50 ` Christophe Leroy
2019-12-09 13:21 ` Andrew Donnellan
2019-12-09 13:26 ` Andrew Donnellan
0 siblings, 2 replies; 5+ messages in thread
From: Christophe Leroy @ 2019-12-09 11:50 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev; +Cc: syzbot+f25ecf4b2982d8c7a640, Daniel Axtens
Le 09/12/2019 à 11:59, Andrew Donnellan a écrit :
> The KUAP implementation adds calls in clear_user() to enable and disable
> access to userspace memory. However, it doesn't add these to
> __clear_user(), which is used in the ptrace regset code.
>
> As there's only one direct user of __clear_user(), and the time taken to
> set the AMR for KUAP purposes is going to dominate the cost of a quick
> access_ok(), there's not much point having a separate path.
No risk that access_ok() fails ?
There is also a call to might_fault() in clear_user(), isn't it a problem ?
>
> Rename __clear_user() to clear_user_asm(), and make __clear_user() just
> call clear_user().
>
> Reported-by: syzbot+f25ecf4b2982d8c7a640@syzkaller-ppc64.appspotmail.com
> Reported-by: Daniel Axtens <dja@axtens.net>
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Russell Currey <ruscur@russell.cc>
> Fixes: de78a9c42a79 ("powerpc: Add a framework for Kernel Userspace Access Protection")
> Signed-off-by: Andrew Donnellan <ajd@linux.ibm.com>
> ---
> arch/powerpc/include/asm/uaccess.h | 9 +++++++--
> arch/powerpc/lib/string_32.S | 4 ++--
> arch/powerpc/lib/string_64.S | 6 +++---
> 3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 15002b51ff18..d05bc0a4cafa 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -401,7 +401,7 @@ copy_to_user_mcsafe(void __user *to, const void *from, unsigned long n)
> return n;
> }
>
> -extern unsigned long __clear_user(void __user *addr, unsigned long size);
> +extern unsigned long clear_user_asm(void __user *addr, unsigned long size);
>
> static inline unsigned long clear_user(void __user *addr, unsigned long size)
> {
> @@ -409,12 +409,17 @@ static inline unsigned long clear_user(void __user *addr, unsigned long size)
> might_fault();
> if (likely(access_ok(addr, size))) {
> allow_write_to_user(addr, size);
> - ret = __clear_user(addr, size);
> + ret = clear_user_asm(addr, size);
> prevent_write_to_user(addr, size);
> }
What about changing the above by the following ?
if (likely(access_ok(addr, size))) ret = __clear_user(addr, size);
> return ret;
> }
>
> +static inline unsigned long __clear_user(void __user *addr, unsigned long size)
> +{
> + return clear_user(addr, size);
> +}
> +
Then
static inline unsigned long __clear_user(void __user *addr, unsigned
long size)
{
allow_write_to_user(addr, size);
ret = clear_user_asm(addr, size);
prevent_write_to_user(addr, size);
return ret;
}
> extern long strncpy_from_user(char *dst, const char __user *src, long count);
> extern __must_check long strnlen_user(const char __user *str, long n);
Christophe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix __clear_user() with KUAP enabled
2019-12-09 11:50 ` Christophe Leroy
@ 2019-12-09 13:21 ` Andrew Donnellan
2019-12-09 13:26 ` Andrew Donnellan
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Donnellan @ 2019-12-09 13:21 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: syzbot+f25ecf4b2982d8c7a640, Daniel Axtens
On 9/12/19 10:50 pm, Christophe Leroy wrote:
>
>
> Le 09/12/2019 à 11:59, Andrew Donnellan a écrit :
>> The KUAP implementation adds calls in clear_user() to enable and disable
>> access to userspace memory. However, it doesn't add these to
>> __clear_user(), which is used in the ptrace regset code.
>>
>> As there's only one direct user of __clear_user(), and the time taken to
>> set the AMR for KUAP purposes is going to dominate the cost of a quick
>> access_ok(), there's not much point having a separate path.
>
> No risk that access_ok() fails ?
>
The only user of __clear_user() is the regset code, and
copy_regset_{to,from}_user() already checks access_ok().
I think ideally we could get rid of __clear_user() completely.
> There is also a call to might_fault() in clear_user(), isn't it a problem ?
I don't think it's a problem, just some lockdep debugging?
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix __clear_user() with KUAP enabled
2019-12-09 11:50 ` Christophe Leroy
2019-12-09 13:21 ` Andrew Donnellan
@ 2019-12-09 13:26 ` Andrew Donnellan
2019-12-09 16:31 ` Christophe Leroy
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Donnellan @ 2019-12-09 13:26 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: syzbot+f25ecf4b2982d8c7a640, Daniel Axtens
On 9/12/19 10:50 pm, Christophe Leroy wrote:
>> -extern unsigned long __clear_user(void __user *addr, unsigned long
>> size);
>> +extern unsigned long clear_user_asm(void __user *addr, unsigned long
>> size);
>> static inline unsigned long clear_user(void __user *addr, unsigned
>> long size)
>> {
>> @@ -409,12 +409,17 @@ static inline unsigned long clear_user(void
>> __user *addr, unsigned long size)
>> might_fault();
>> if (likely(access_ok(addr, size))) {
>> allow_write_to_user(addr, size);
>> - ret = __clear_user(addr, size);
>> + ret = clear_user_asm(addr, size);
>> prevent_write_to_user(addr, size);
>> }
>
> What about changing the above by the following ?
>
> if (likely(access_ok(addr, size))) ret =
> __clear_user(addr, size);
>
>> return ret;
>> }
>> +static inline unsigned long __clear_user(void __user *addr, unsigned
>> long size)
>> +{
>> + return clear_user(addr, size);
>> +}
>> +
>
> Then
>
> static inline unsigned long __clear_user(void __user *addr, unsigned
> long size)
> {
> allow_write_to_user(addr, size);
> ret = clear_user_asm(addr, size);
> prevent_write_to_user(addr, size);
>
> return ret;
> }
This is exactly the patch I initially wrote, I ran it past mpe privately
and he suggested aliasing clear_user() -> __clear_user() instead, as
there's not much point keeping a separate path around for a single user
for a basically non-existent performance gain.
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc: Fix __clear_user() with KUAP enabled
2019-12-09 13:26 ` Andrew Donnellan
@ 2019-12-09 16:31 ` Christophe Leroy
0 siblings, 0 replies; 5+ messages in thread
From: Christophe Leroy @ 2019-12-09 16:31 UTC (permalink / raw)
To: Andrew Donnellan, linuxppc-dev; +Cc: syzbot+f25ecf4b2982d8c7a640, Daniel Axtens
Le 09/12/2019 à 14:26, Andrew Donnellan a écrit :
> On 9/12/19 10:50 pm, Christophe Leroy wrote:
>>> -extern unsigned long __clear_user(void __user *addr, unsigned long
>>> size);
>>> +extern unsigned long clear_user_asm(void __user *addr, unsigned long
>>> size);
>>> static inline unsigned long clear_user(void __user *addr, unsigned
>>> long size)
>>> {
>>> @@ -409,12 +409,17 @@ static inline unsigned long clear_user(void
>>> __user *addr, unsigned long size)
>>> might_fault();
>>> if (likely(access_ok(addr, size))) {
>>> allow_write_to_user(addr, size);
>>> - ret = __clear_user(addr, size);
>>> + ret = clear_user_asm(addr, size);
>>> prevent_write_to_user(addr, size);
>>> }
>>
>> What about changing the above by the following ?
>>
>> if (likely(access_ok(addr, size))) ret =
>> __clear_user(addr, size);
>>
>>> return ret;
>>> }
>>> +static inline unsigned long __clear_user(void __user *addr, unsigned
>>> long size)
>>> +{
>>> + return clear_user(addr, size);
>>> +}
>>> +
>>
>> Then
>>
>> static inline unsigned long __clear_user(void __user *addr, unsigned
>> long size)
>> {
>> allow_write_to_user(addr, size);
>> ret = clear_user_asm(addr, size);
>> prevent_write_to_user(addr, size);
>>
>> return ret;
>> }
>
> This is exactly the patch I initially wrote, I ran it past mpe privately
> and he suggested aliasing clear_user() -> __clear_user() instead, as
> there's not much point keeping a separate path around for a single user
> for a basically non-existent performance gain.
>
But other arches do call __clear_user() from clear_user() and do the
access unlock / lock in __clear_user() (at least arm and x86/64).
I think it would be better to do the same way as other arches,
especially as __clear_user() can be called from anywhere in the kernel,
so I'd expect it to behave the same on all arches.
I don't see it as the separate path, to me it looks more like code
refactoring. And it follows the same logic as many other functions in
the kernel: __the_said_function() is the light version of
the_said_function().
Christophe
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-09 16:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09 10:59 [PATCH] powerpc: Fix __clear_user() with KUAP enabled Andrew Donnellan
2019-12-09 11:50 ` Christophe Leroy
2019-12-09 13:21 ` Andrew Donnellan
2019-12-09 13:26 ` Andrew Donnellan
2019-12-09 16:31 ` Christophe Leroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).