* [PATCH 2/2] powerpc: use probe_user_read() and probe_user_address()
2018-12-03 17:06 [PATCH 1/2] mm: add probe_user_read() and probe_user_address() Christophe Leroy
@ 2018-12-03 17:06 ` Christophe Leroy
2018-12-03 17:53 ` [PATCH 1/2] mm: add " Mike Rapoport
2018-12-04 8:42 ` Michael Ellerman
2 siblings, 0 replies; 4+ messages in thread
From: Christophe Leroy @ 2018-12-03 17:06 UTC (permalink / raw)
To: Kees Cook, Andrew Morton, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linux-kernel, linuxppc-dev, linux-mm
Instead of opencoding, use probe_user_read() and probe_user_address()
to failessly read a user location.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/process.c | 12 +-----------
arch/powerpc/mm/fault.c | 6 +-----
arch/powerpc/perf/callchain.c | 20 +++-----------------
arch/powerpc/perf/core-book3s.c | 8 +-------
arch/powerpc/sysdev/fsl_pci.c | 9 +++------
5 files changed, 9 insertions(+), 46 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 96f34730010f..b77a2b4007e5 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1298,16 +1298,6 @@ void show_user_instructions(struct pt_regs *regs)
pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
- /*
- * Make sure the NIP points at userspace, not kernel text/data or
- * elsewhere.
- */
- if (!__access_ok(pc, NR_INSN_TO_PRINT * sizeof(int), USER_DS)) {
- pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
- current->comm, current->pid);
- return;
- }
-
seq_buf_init(&s, buf, sizeof(buf));
while (n) {
@@ -1318,7 +1308,7 @@ void show_user_instructions(struct pt_regs *regs)
for (i = 0; i < 8 && n; i++, n--, pc += sizeof(int)) {
int instr;
- if (probe_kernel_address((const void *)pc, instr)) {
+ if (probe_user_address((const void *)pc, instr)) {
seq_buf_printf(&s, "XXXXXXXX ");
continue;
}
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 1697e903bbf2..a87a2d2e7a8b 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -274,12 +274,8 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
access_ok(VERIFY_READ, nip, sizeof(*nip))) {
unsigned int inst;
- int res;
- pagefault_disable();
- res = __get_user_inatomic(inst, nip);
- pagefault_enable();
- if (!res)
+ if (!probe_user_address(nip, inst))
return !store_updates_sp(inst);
*must_retry = true;
}
diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
index 0af051a1974e..efe518c4e8c7 100644
--- a/arch/powerpc/perf/callchain.c
+++ b/arch/powerpc/perf/callchain.c
@@ -159,12 +159,8 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
((unsigned long)ptr & 7))
return -EFAULT;
- pagefault_disable();
- if (!__get_user_inatomic(*ret, ptr)) {
- pagefault_enable();
+ if (!probe_user_address(ptr, *ret))
return 0;
- }
- pagefault_enable();
return read_user_stack_slow(ptr, ret, 8);
}
@@ -175,12 +171,8 @@ static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
((unsigned long)ptr & 3))
return -EFAULT;
- pagefault_disable();
- if (!__get_user_inatomic(*ret, ptr)) {
- pagefault_enable();
+ if (!probe_user_address(ptr, *ret))
return 0;
- }
- pagefault_enable();
return read_user_stack_slow(ptr, ret, 4);
}
@@ -307,17 +299,11 @@ static inline int current_is_64bit(void)
*/
static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
{
- int rc;
-
if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
((unsigned long)ptr & 3))
return -EFAULT;
- pagefault_disable();
- rc = __get_user_inatomic(*ret, ptr);
- pagefault_enable();
-
- return rc;
+ return probe_user_address(ptr, *ret);
}
static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 81f8a0c838ae..25b28aa499b9 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -407,7 +407,6 @@ static void power_pmu_sched_task(struct perf_event_context *ctx, bool sched_in)
static __u64 power_pmu_bhrb_to(u64 addr)
{
unsigned int instr;
- int ret;
__u64 target;
if (is_kernel_addr(addr)) {
@@ -418,13 +417,8 @@ static __u64 power_pmu_bhrb_to(u64 addr)
}
/* Userspace: need copy instruction here then translate it */
- pagefault_disable();
- ret = __get_user_inatomic(instr, (unsigned int __user *)addr);
- if (ret) {
- pagefault_enable();
+ if (probe_user_address((unsigned int __user *)addr, instr))
return 0;
- }
- pagefault_enable();
target = branch_target(&instr);
if ((!target) || (instr & BRANCH_ABSOLUTE))
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 918be816b097..b8e2349910b7 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -1068,13 +1068,10 @@ int fsl_pci_mcheck_exception(struct pt_regs *regs)
addr += mfspr(SPRN_MCAR);
if (is_in_pci_mem_space(addr)) {
- if (user_mode(regs)) {
- pagefault_disable();
- ret = get_user(inst, (__u32 __user *)regs->nip);
- pagefault_enable();
- } else {
+ if (user_mode(regs))
+ ret = probe_user_address((void *)regs->nip, inst);
+ else
ret = probe_kernel_address((void *)regs->nip, inst);
- }
if (!ret && mcheck_handle_load(regs, inst)) {
regs->nip += 4;
--
2.13.3
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] mm: add probe_user_read() and probe_user_address()
2018-12-03 17:06 [PATCH 1/2] mm: add probe_user_read() and probe_user_address() Christophe Leroy
2018-12-03 17:06 ` [PATCH 2/2] powerpc: use " Christophe Leroy
@ 2018-12-03 17:53 ` Mike Rapoport
2018-12-04 8:42 ` Michael Ellerman
2 siblings, 0 replies; 4+ messages in thread
From: Mike Rapoport @ 2018-12-03 17:53 UTC (permalink / raw)
To: Christophe Leroy
Cc: Kees Cook, Andrew Morton, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, linux-kernel, linuxppc-dev, linux-mm
On Mon, Dec 03, 2018 at 05:06:42PM +0000, Christophe Leroy wrote:
> In the powerpc, there are several places implementing safe
> access to user data. This is sometimes implemented using
> probe_kernel_address() with additional access_ok() verification,
> sometimes with get_user() enclosed in a pagefault_disable()/enable()
> pair, etc... :
> show_user_instructions()
> bad_stack_expansion()
> p9_hmi_special_emu()
> fsl_pci_mcheck_exception()
> read_user_stack_64()
> read_user_stack_32() on PPC64
> read_user_stack_32() on PPC32
> power_pmu_bhrb_to()
>
> In the same spirit as probe_kernel_read() and probe_kernel_address(),
> this patch adds probe_user_read() and probe_user_address().
>
> probe_user_read() does the same as probe_kernel_read() but
> first checks that it is really a user address.
>
> probe_user_address() is a shortcut to probe_user_read()
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> Changes since RFC: Made a static inline function instead of weak function as recommended by Kees.
>
> include/linux/uaccess.h | 42 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index efe79c1cdd47..83ea8aefca75 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -266,6 +266,48 @@ extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
> #define probe_kernel_address(addr, retval) \
> probe_kernel_read(&retval, addr, sizeof(retval))
>
> +/**
> + * probe_user_read(): safely attempt to read from a user location
> + * @dst: pointer to the buffer that shall take the data
> + * @src: address to read from
> + * @size: size of the data chunk
> + *
> + * Safely read from address @src to the buffer at @dst. If a kernel fault
> + * happens, handle that and return -EFAULT.
> + *
> + * We ensure that the copy_from_user is executed in atomic context so that
> + * do_page_fault() doesn't attempt to take mmap_sem. This makes
> + * probe_user_read() suitable for use within regions where the caller
> + * already holds mmap_sem, or other locks which nest inside mmap_sem.
Please add 'Returns:' description.
> + */
> +
> +#ifndef probe_user_read
> +static __always_inline long probe_user_read(void *dst, const void __user *src,
> + size_t size)
> +{
> + long ret;
> +
> + if (!access_ok(VERIFY_READ, src, size))
> + return -EFAULT;
> +
> + pagefault_disable();
> + ret = __copy_from_user_inatomic(dst, src, size);
> + pagefault_enable();
> +
> + return ret ? -EFAULT : 0;
> +}
> +#endif
> +
> +/**
> + * probe_user_address(): safely attempt to read from a user location
> + * @addr: address to read from
> + * @retval: read into this variable
> + *
> + * Returns 0 on success, or -EFAULT.
This should be 'Return:' for kernel-doc to recognise it as return value
description.
> + */
> +#define probe_user_address(addr, retval) \
> + probe_user_read(&(retval), addr, sizeof(retval))
> +
> #ifndef user_access_begin
> #define user_access_begin() do { } while (0)
> #define user_access_end() do { } while (0)
> --
> 2.13.3
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] mm: add probe_user_read() and probe_user_address()
2018-12-03 17:06 [PATCH 1/2] mm: add probe_user_read() and probe_user_address() Christophe Leroy
2018-12-03 17:06 ` [PATCH 2/2] powerpc: use " Christophe Leroy
2018-12-03 17:53 ` [PATCH 1/2] mm: add " Mike Rapoport
@ 2018-12-04 8:42 ` Michael Ellerman
2 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2018-12-04 8:42 UTC (permalink / raw)
To: Christophe Leroy, Kees Cook, Andrew Morton,
Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-kernel, linuxppc-dev, linux-mm
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> In the powerpc, there are several places implementing safe
^
code ?
> access to user data. This is sometimes implemented using
> probe_kernel_address() with additional access_ok() verification,
> sometimes with get_user() enclosed in a pagefault_disable()/enable()
> pair, etc... :
> show_user_instructions()
> bad_stack_expansion()
> p9_hmi_special_emu()
> fsl_pci_mcheck_exception()
> read_user_stack_64()
> read_user_stack_32() on PPC64
> read_user_stack_32() on PPC32
> power_pmu_bhrb_to()
>
> In the same spirit as probe_kernel_read() and probe_kernel_address(),
> this patch adds probe_user_read() and probe_user_address().
>
> probe_user_read() does the same as probe_kernel_read() but
> first checks that it is really a user address.
>
> probe_user_address() is a shortcut to probe_user_read()
...
> +#define probe_user_address(addr, retval) \
> + probe_user_read(&(retval), addr, sizeof(retval))
I realise you added probe_user_address() to mirror probe_kernel_address(),
but I'd rather we just used probe_user_read() directly.
The only advantage of probe_kernel_address() is that you don't have to
mention retval twice.
But the downsides are that it's not obvious that you're writing to
retval (because the macro takes the address for you), and retval is
evaluated twice (the latter is usually not a problem but it can be).
eg, call sites like this are confusing:
static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
{
...
if (!probe_user_address(ptr, *ret))
return 0;
It's confusing because ret is a pointer, but then we dereference it
before passing it to probe_user_address(), so it looks like we're just
passing a value, but we're not.
Compare to:
if (!probe_user_read(ret, ptr, sizeof(*ret)))
return 0;
Which is entirely analogous to a call to memcpy() and involves no magic.
I know there's lots of precedent here with get_user() etc. but that
doesn't mean we have to follow that precedent blindly :)
I guess perhaps we can add probe_user_address() but just not use it in
the powerpc code, if other folks want it to exist.
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread