linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] mm: add probe_user_read()
@ 2019-01-08  7:37 Christophe Leroy
  2019-01-08  7:37 ` [PATCH v2 2/2] powerpc: use probe_user_read() Christophe Leroy
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Christophe Leroy @ 2019-01-08  7:37 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton, Benjamin Herrenschmidt, Paul Mackerras,
	Michael Ellerman, Mike Rapoport
  Cc: linux-mm, linuxppc-dev, linux-kernel

In powerpc code, 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(), this patch adds
probe_user_read().

probe_user_read() does the same as probe_kernel_read() but
first checks that it is really a user address.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 v2: Added "Returns:" comment and removed probe_user_address()

 Changes since RFC: Made a static inline function instead of weak function as recommended by Kees.

 include/linux/uaccess.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 37b226e8df13..07f4f0ed69bc 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -263,6 +263,40 @@ 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
+ *
+ * Returns: 0 on success, -EFAULT on error.
+ *
+ * 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.
+ */
+
+#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(src, size))
+		return -EFAULT;
+
+	pagefault_disable();
+	ret = __copy_from_user_inatomic(dst, src, size);
+	pagefault_enable();
+
+	return ret ? -EFAULT : 0;
+}
+#endif
+
 #ifndef user_access_begin
 #define user_access_begin(ptr,len) access_ok(ptr, len)
 #define user_access_end() do { } while (0)
-- 
2.13.3


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] powerpc: use probe_user_read()
  2019-01-08  7:37 [PATCH v2 1/2] mm: add probe_user_read() Christophe Leroy
@ 2019-01-08  7:37 ` Christophe Leroy
  2019-01-08  9:04   ` David Hildenbrand
  2019-01-08  9:37   ` Christophe Leroy
  2019-01-08  7:51 ` [PATCH v2 1/2] mm: add probe_user_read() Mike Rapoport
  2019-01-08 19:48 ` Andrew Morton
  2 siblings, 2 replies; 10+ messages in thread
From: Christophe Leroy @ 2019-01-08  7:37 UTC (permalink / raw)
  To: Kees Cook, Andrew Morton, Benjamin Herrenschmidt, Paul Mackerras,
	Michael Ellerman, Mike Rapoport
  Cc: linux-mm, linuxppc-dev, linux-kernel

Instead of opencoding, use probe_user_read() to failessly
read a user location.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 v2: Using probe_user_read() instead of probe_user_address()

 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   | 10 ++++------
 5 files changed, 10 insertions(+), 46 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index ce393df243aa..6a4b59d574c2 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_read(&instr, (void __user *)pc, sizeof(instr))) {
 				seq_buf_printf(&s, "XXXXXXXX ");
 				continue;
 			}
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 887f11bcf330..ec74305fa330 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -276,12 +276,8 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
 		if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
 		    access_ok(nip, sizeof(*nip))) {
 			unsigned int inst;
-			int res;
 
-			pagefault_disable();
-			res = __get_user_inatomic(inst, nip);
-			pagefault_enable();
-			if (!res)
+			if (!probe_user_read(&inst, nip, sizeof(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..0680efb2237b 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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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 b0723002a396..4b64ddf0db68 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -416,7 +416,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)) {
@@ -427,13 +426,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_read(&instr, (unsigned int __user *)addr, sizeof(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..c8a1b26489f5 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -1068,13 +1068,11 @@ 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_read(&inst, (void __user *)regs->nip,
+					      sizeof(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 related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] mm: add probe_user_read()
  2019-01-08  7:37 [PATCH v2 1/2] mm: add probe_user_read() Christophe Leroy
  2019-01-08  7:37 ` [PATCH v2 2/2] powerpc: use probe_user_read() Christophe Leroy
@ 2019-01-08  7:51 ` Mike Rapoport
  2019-01-08 19:48 ` Andrew Morton
  2 siblings, 0 replies; 10+ messages in thread
From: Mike Rapoport @ 2019-01-08  7:51 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Kees Cook, linux-kernel, linux-mm, Paul Mackerras, Andrew Morton,
	linuxppc-dev

On Tue, Jan 08, 2019 at 07:37:44AM +0000, Christophe Leroy wrote:
> In powerpc code, 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(), this patch adds
> probe_user_read().
> 
> probe_user_read() does the same as probe_kernel_read() but
> first checks that it is really a user address.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  v2: Added "Returns:" comment and removed probe_user_address()
> 
>  Changes since RFC: Made a static inline function instead of weak function as recommended by Kees.
> 
>  include/linux/uaccess.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 37b226e8df13..07f4f0ed69bc 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -263,6 +263,40 @@ 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
> + *
> + * Returns: 0 on success, -EFAULT on error.

Nit: please put the "Returns:" comment after the description, otherwise
kernel-doc considers it a part of the elaborate description.

> + *
> + * 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.
> + */
> +
> +#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(src, size))
> +		return -EFAULT;
> +
> +	pagefault_disable();
> +	ret = __copy_from_user_inatomic(dst, src, size);
> +	pagefault_enable();
> +
> +	return ret ? -EFAULT : 0;
> +}
> +#endif
> +
>  #ifndef user_access_begin
>  #define user_access_begin(ptr,len) access_ok(ptr, len)
>  #define user_access_end() do { } while (0)
> -- 
> 2.13.3
> 

-- 
Sincerely yours,
Mike.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] powerpc: use probe_user_read()
  2019-01-08  7:37 ` [PATCH v2 2/2] powerpc: use probe_user_read() Christophe Leroy
@ 2019-01-08  9:04   ` David Hildenbrand
  2019-01-08  9:19     ` Christophe Leroy
  2019-01-08  9:37   ` Christophe Leroy
  1 sibling, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2019-01-08  9:04 UTC (permalink / raw)
  To: Christophe Leroy, Kees Cook, Andrew Morton,
	Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Mike Rapoport
  Cc: linux-mm, linuxppc-dev, linux-kernel

On 08.01.19 08:37, Christophe Leroy wrote:
> Instead of opencoding, use probe_user_read() to failessly
> read a user location.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  v2: Using probe_user_read() instead of probe_user_address()
> 
>  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   | 10 ++++------
>  5 files changed, 10 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index ce393df243aa..6a4b59d574c2 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_read(&instr, (void __user *)pc, sizeof(instr))) {
>  				seq_buf_printf(&s, "XXXXXXXX ");
>  				continue;
>  			}
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 887f11bcf330..ec74305fa330 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -276,12 +276,8 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
>  		if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
>  		    access_ok(nip, sizeof(*nip))) {
>  			unsigned int inst;
> -			int res;
>  
> -			pagefault_disable();
> -			res = __get_user_inatomic(inst, nip);
> -			pagefault_enable();
> -			if (!res)
> +			if (!probe_user_read(&inst, nip, sizeof(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..0680efb2237b 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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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 b0723002a396..4b64ddf0db68 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -416,7 +416,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)) {
> @@ -427,13 +426,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_read(&instr, (unsigned int __user *)addr, sizeof(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..c8a1b26489f5 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -1068,13 +1068,11 @@ 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_read(&inst, (void __user *)regs->nip,
> +					      sizeof(inst));

What about also adding probe_user_address ?

> +		else
>  			ret = probe_kernel_address((void *)regs->nip, inst);
> -		}
>  
>  		if (!ret && mcheck_handle_load(regs, inst)) {
>  			regs->nip += 4;
> 


-- 

Thanks,

David / dhildenb

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] powerpc: use probe_user_read()
  2019-01-08  9:04   ` David Hildenbrand
@ 2019-01-08  9:19     ` Christophe Leroy
  0 siblings, 0 replies; 10+ messages in thread
From: Christophe Leroy @ 2019-01-08  9:19 UTC (permalink / raw)
  To: David Hildenbrand, Kees Cook, Andrew Morton,
	Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Mike Rapoport
  Cc: linux-mm, linuxppc-dev, linux-kernel



Le 08/01/2019 à 10:04, David Hildenbrand a écrit :
> On 08.01.19 08:37, Christophe Leroy wrote:
>> Instead of opencoding, use probe_user_read() to failessly
>> read a user location.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   v2: Using probe_user_read() instead of probe_user_address()
>>
>>   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   | 10 ++++------
>>   5 files changed, 10 insertions(+), 46 deletions(-)
>>

[snip]

>> diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
>> index 918be816b097..c8a1b26489f5 100644
>> --- a/arch/powerpc/sysdev/fsl_pci.c
>> +++ b/arch/powerpc/sysdev/fsl_pci.c
>> @@ -1068,13 +1068,11 @@ 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_read(&inst, (void __user *)regs->nip,
>> +					      sizeof(inst));
> 
> What about also adding probe_user_address ?

Michael doesn't like it, see https://patchwork.ozlabs.org/patch/1007117/

Christophe

> 
>> +		else
>>   			ret = probe_kernel_address((void *)regs->nip, inst);
>> -		}
>>   
>>   		if (!ret && mcheck_handle_load(regs, inst)) {
>>   			regs->nip += 4;
>>
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] powerpc: use probe_user_read()
  2019-01-08  7:37 ` [PATCH v2 2/2] powerpc: use probe_user_read() Christophe Leroy
  2019-01-08  9:04   ` David Hildenbrand
@ 2019-01-08  9:37   ` Christophe Leroy
  2019-01-08  9:58     ` Russell Currey
  1 sibling, 1 reply; 10+ messages in thread
From: Christophe Leroy @ 2019-01-08  9:37 UTC (permalink / raw)
  To: Michael Ellerman, Russell Currey
  Cc: Kees Cook, linux-kernel, Mike Rapoport, linux-mm, Paul Mackerras,
	Andrew Morton, linuxppc-dev

Hi Michael and Russell,

Any idea why:
- checkpatch reports missing Signed-off-by:
- Snowpatch build fails on PPC64 (it seems unrelated to the patch, 
something wrong in lib/genalloc.c)

Thanks
Christophe

Le 08/01/2019 à 08:37, Christophe Leroy a écrit :
> Instead of opencoding, use probe_user_read() to failessly
> read a user location.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>   v2: Using probe_user_read() instead of probe_user_address()
> 
>   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   | 10 ++++------
>   5 files changed, 10 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index ce393df243aa..6a4b59d574c2 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_read(&instr, (void __user *)pc, sizeof(instr))) {
>   				seq_buf_printf(&s, "XXXXXXXX ");
>   				continue;
>   			}
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 887f11bcf330..ec74305fa330 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -276,12 +276,8 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
>   		if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
>   		    access_ok(nip, sizeof(*nip))) {
>   			unsigned int inst;
> -			int res;
>   
> -			pagefault_disable();
> -			res = __get_user_inatomic(inst, nip);
> -			pagefault_enable();
> -			if (!res)
> +			if (!probe_user_read(&inst, nip, sizeof(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..0680efb2237b 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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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 b0723002a396..4b64ddf0db68 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -416,7 +416,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)) {
> @@ -427,13 +426,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_read(&instr, (unsigned int __user *)addr, sizeof(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..c8a1b26489f5 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -1068,13 +1068,11 @@ 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_read(&inst, (void __user *)regs->nip,
> +					      sizeof(inst));
> +		else
>   			ret = probe_kernel_address((void *)regs->nip, inst);
> -		}
>   
>   		if (!ret && mcheck_handle_load(regs, inst)) {
>   			regs->nip += 4;
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] powerpc: use probe_user_read()
  2019-01-08  9:37   ` Christophe Leroy
@ 2019-01-08  9:58     ` Russell Currey
  0 siblings, 0 replies; 10+ messages in thread
From: Russell Currey @ 2019-01-08  9:58 UTC (permalink / raw)
  To: Christophe Leroy, Michael Ellerman
  Cc: Kees Cook, linux-kernel, Mike Rapoport, linux-mm, Paul Mackerras,
	Andrew Morton, linuxppc-dev

On Tue, 2019-01-08 at 10:37 +0100, Christophe Leroy wrote:
> Hi Michael and Russell,
> 
> Any idea why:
> - checkpatch reports missing Signed-off-by:
> - Snowpatch build fails on PPC64 (it seems unrelated to the patch, 
> something wrong in lib/genalloc.c)

Upstream kernel broke for powerpc (snowpatch applies patches on top of
powerpc/next), it was fixed in commit
35004f2e55807a1a1491db24ab512dd2f770a130 which I believe is in
powerpc/next now.  I will look at rerunning tests for all the patches
that this impacted.

As for the S-o-b, no clue, I'll have a look.  Thanks for the report!

- Russell

> 
> Thanks
> Christophe
> 
> Le 08/01/2019 à 08:37, Christophe Leroy a écrit :
> > Instead of opencoding, use probe_user_read() to failessly
> > read a user location.
> > 
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > ---
> >   v2: Using probe_user_read() instead of probe_user_address()
> > 
> >   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   | 10 ++++------
> >   5 files changed, 10 insertions(+), 46 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/process.c
> > b/arch/powerpc/kernel/process.c
> > index ce393df243aa..6a4b59d574c2 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_read(&instr, (void __user *)pc,
> > sizeof(instr))) {
> >   				seq_buf_printf(&s, "XXXXXXXX ");
> >   				continue;
> >   			}
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 887f11bcf330..ec74305fa330 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -276,12 +276,8 @@ static bool bad_stack_expansion(struct pt_regs
> > *regs, unsigned long address,
> >   		if ((flags & FAULT_FLAG_WRITE) && (flags &
> > FAULT_FLAG_USER) &&
> >   		    access_ok(nip, sizeof(*nip))) {
> >   			unsigned int inst;
> > -			int res;
> >   
> > -			pagefault_disable();
> > -			res = __get_user_inatomic(inst, nip);
> > -			pagefault_enable();
> > -			if (!res)
> > +			if (!probe_user_read(&inst, nip, sizeof(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..0680efb2237b 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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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_read(ret, ptr, sizeof(*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 b0723002a396..4b64ddf0db68 100644
> > --- a/arch/powerpc/perf/core-book3s.c
> > +++ b/arch/powerpc/perf/core-book3s.c
> > @@ -416,7 +416,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)) {
> > @@ -427,13 +426,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_read(&instr, (unsigned int __user *)addr,
> > sizeof(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..c8a1b26489f5 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -1068,13 +1068,11 @@ 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_read(&inst, (void __user
> > *)regs->nip,
> > +					      sizeof(inst));
> > +		else
> >   			ret = probe_kernel_address((void *)regs->nip,
> > inst);
> > -		}
> >   
> >   		if (!ret && mcheck_handle_load(regs, inst)) {
> >   			regs->nip += 4;
> > 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] mm: add probe_user_read()
  2019-01-08  7:37 [PATCH v2 1/2] mm: add probe_user_read() Christophe Leroy
  2019-01-08  7:37 ` [PATCH v2 2/2] powerpc: use probe_user_read() Christophe Leroy
  2019-01-08  7:51 ` [PATCH v2 1/2] mm: add probe_user_read() Mike Rapoport
@ 2019-01-08 19:48 ` Andrew Morton
  2019-01-08 21:11   ` Christophe Leroy
  2 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2019-01-08 19:48 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Kees Cook, linux-kernel, Mike Rapoport, linux-mm, Paul Mackerras,
	linuxppc-dev

On Tue,  8 Jan 2019 07:37:44 +0000 (UTC) Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> In powerpc code, 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(), this patch adds
> probe_user_read().
> 
> probe_user_read() does the same as probe_kernel_read() but
> first checks that it is really a user address.
> 
> ...
>
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -263,6 +263,40 @@ 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
> + *
> + * Returns: 0 on success, -EFAULT on error.
> + *
> + * 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.
> + */
> +
> +#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(src, size))
> +		return -EFAULT;
> +
> +	pagefault_disable();
> +	ret = __copy_from_user_inatomic(dst, src, size);
> +	pagefault_enable();
> +
> +	return ret ? -EFAULT : 0;
> +}
> +#endif

Why was the __always_inline needed?

This function is pretty large.  Why is it inlined?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] mm: add probe_user_read()
  2019-01-08 19:48 ` Andrew Morton
@ 2019-01-08 21:11   ` Christophe Leroy
  2019-01-08 21:14     ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Christophe Leroy @ 2019-01-08 21:11 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Kees Cook, linux-kernel, Mike Rapoport, linux-mm, Paul Mackerras,
	linuxppc-dev



Le 08/01/2019 à 20:48, Andrew Morton a écrit :
> On Tue,  8 Jan 2019 07:37:44 +0000 (UTC) Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> 
>> In powerpc code, 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(), this patch adds
>> probe_user_read().
>>
>> probe_user_read() does the same as probe_kernel_read() but
>> first checks that it is really a user address.
>>
>> ...
>>
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -263,6 +263,40 @@ 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
>> + *
>> + * Returns: 0 on success, -EFAULT on error.
>> + *
>> + * 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.
>> + */
>> +
>> +#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(src, size))
>> +		return -EFAULT;
>> +
>> +	pagefault_disable();
>> +	ret = __copy_from_user_inatomic(dst, src, size);
>> +	pagefault_enable();
>> +
>> +	return ret ? -EFAULT : 0;
>> +}
>> +#endif
> 
> Why was the __always_inline needed?
> 
> This function is pretty large.  Why is it inlined?
> 

Kees told to do that way, see https://patchwork.ozlabs.org/patch/986848/

Christophe

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] mm: add probe_user_read()
  2019-01-08 21:11   ` Christophe Leroy
@ 2019-01-08 21:14     ` Kees Cook
  0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2019-01-08 21:14 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: LKML, Mike Rapoport, Linux-MM, Paul Mackerras, Andrew Morton, PowerPC

On Tue, Jan 8, 2019 at 1:11 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 08/01/2019 à 20:48, Andrew Morton a écrit :
> > On Tue,  8 Jan 2019 07:37:44 +0000 (UTC) Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >> In powerpc code, 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(), this patch adds
> >> probe_user_read().
> >>
> >> probe_user_read() does the same as probe_kernel_read() but
> >> first checks that it is really a user address.
> >>
> >> ...
> >>
> >> --- a/include/linux/uaccess.h
> >> +++ b/include/linux/uaccess.h
> >> @@ -263,6 +263,40 @@ 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
> >> + *
> >> + * Returns: 0 on success, -EFAULT on error.
> >> + *
> >> + * 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.
> >> + */
> >> +
> >> +#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(src, size))
> >> +            return -EFAULT;
> >> +
> >> +    pagefault_disable();
> >> +    ret = __copy_from_user_inatomic(dst, src, size);
> >> +    pagefault_enable();
> >> +
> >> +    return ret ? -EFAULT : 0;
> >> +}
> >> +#endif
> >
> > Why was the __always_inline needed?
> >
> > This function is pretty large.  Why is it inlined?
> >
>
> Kees told to do that way, see https://patchwork.ozlabs.org/patch/986848/

Yeah, I'd like to make sure we can plumb the size checks down into the
user copy primitives.

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-01-08 21:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-08  7:37 [PATCH v2 1/2] mm: add probe_user_read() Christophe Leroy
2019-01-08  7:37 ` [PATCH v2 2/2] powerpc: use probe_user_read() Christophe Leroy
2019-01-08  9:04   ` David Hildenbrand
2019-01-08  9:19     ` Christophe Leroy
2019-01-08  9:37   ` Christophe Leroy
2019-01-08  9:58     ` Russell Currey
2019-01-08  7:51 ` [PATCH v2 1/2] mm: add probe_user_read() Mike Rapoport
2019-01-08 19:48 ` Andrew Morton
2019-01-08 21:11   ` Christophe Leroy
2019-01-08 21:14     ` Kees Cook

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