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+f25ecf4b2982d8c7a...@syzkaller-ppc64.appspotmail.com
Reported-by: Daniel Axtens <d...@axtens.net>
Suggested-by: Michael Ellerman <m...@ellerman.id.au>
Cc: Christophe Leroy <christophe.le...@c-s.fr>
Cc: Russell Currey <rus...@russell.cc>
Fixes: de78a9c42a79 ("powerpc: Add a framework for Kernel Userspace Access
Protection")
Signed-off-by: Andrew Donnellan <a...@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
