[1/2] mm: add probe_user_read() and probe_user_address()

Commit Message

Christophe Leroy Dec. 3, 2018, 5:06 p.m. UTC

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 <[e-mail]>
---
 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(+)

Comments

Mike Rapoport Dec. 3, 2018, 5:53 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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
>

Michael Ellerman Dec. 4, 2018, 8:42 a.m. UTC | #2

Christophe Leroy <[e-mail]> 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

Patch

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.
+ */
+
+#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.
+ */
+#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)