linux-kernel - Re: [RFC PATCH 2/4] uaccess: Add non-pagefault user-space read functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190225150603.GE32494@hirez.programming.kicks-ass.net>
Date:   Mon, 25 Feb 2019 16:06:03 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Andy Lutomirski <luto@...capital.net>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Changbin Du <changbin.du@...il.com>,
        Jann Horn <jannh@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Nadav Amit <namit@...are.com>
Subject: Re: [RFC PATCH 2/4] uaccess: Add non-pagefault user-space read
 functions

On Mon, Feb 25, 2019 at 11:05:41PM +0900, Masami Hiramatsu wrote:

> +static __always_inline long __strncpy_from_unsafe_user(char *dst,
> +			 const char __user *unsafe_addr, long count)
> +{
> +	if (!access_ok(unsafe_addr, count))
> +		return -EFAULT;
> +
> +	return strncpy_from_unsafe_common(dst, unsafe_addr, count);
> +}

Would something like so work for people?

---
 arch/x86/include/asm/uaccess.h |  8 +++++++-
 include/linux/uaccess.h        | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 780f2b42c8ef..3125d129d3b6 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -92,12 +92,18 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
  * checks that the pointer is in the user space range - after calling
  * this function, memory access functions may still return -EFAULT.
  */
-#define access_ok(addr, size)					\
+#define access_ok(addr, size)						\
 ({									\
 	WARN_ON_IN_IRQ();						\
 	likely(!__range_not_ok(addr, size, user_addr_max()));		\
 })
 
+#define user_access_ok(addr, size)					\
+({									\
+	WARN_ON_ONCE(!segment_eq(get_fs(), USER_DS));			\
+	likely(!__range_not_ok(addr, size, user_addr_max()));		\
+})
+
 /*
  * These are the main single-value transfer routines.  They automatically
  * use the right size if we just have the right pointer type.
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 37b226e8df13..088f2ae09e14 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -10,6 +10,24 @@
 
 #include <asm/uaccess.h>
 
+/**
+ * user_access_ok: Checks if a user space pointer is valid
+ * @addr: User space pointer to start of block to check
+ * @size: Size of block to check
+ *
+ * Context: User context or explicit set_fs(USER_DS).
+ *
+ * This function is very much like access_ok(), except it (may) have different
+ * context validation. In general we must be very careful when using this.
+ */
+#ifndef user_access_ok
+#define user_access_ok(addr, size)					\
+({									\
+	WARN_ON_ONCE(!segment_eq(get_fs(), USER_DS));			\
+	access_ok(addr, size);						\
+})
+#endif
+
 /*
  * Architectures should provide two primitives (raw_copy_{to,from}_user())
  * and get rid of their private instances of copy_{to,from}_user() and

> +/**
> + * strncpy_from_unsafe_user: - Copy a NUL terminated string from unsafe user
> + *				address.
> + * @dst:   Destination address, in kernel space.  This buffer must be at
> + *         least @count bytes long.
> + * @unsafe_addr: Unsafe user address.
> + * @count: Maximum number of bytes to copy, including the trailing NUL.
> + *
> + * Copies a NUL-terminated string from unsafe user address to kernel buffer.
> + *
> + * On success, returns the length of the string INCLUDING the trailing NUL.
> + *
> + * If access fails, returns -EFAULT (some data may have been copied
> + * and the trailing NUL added).
> + *
> + * If @count is smaller than the length of the string, copies @count-1 bytes,
> + * sets the last byte of @dst buffer to NUL and returns @count.
> + */
> +long strncpy_from_unsafe_user(char *dst, const void __user *unsafe_addr,
> +			      long count)
> +{
> +	mm_segment_t old_fs = get_fs();
> +	long ret;
> +
> +	if (unlikely(count <= 0))
> +		return 0;
> +
> +	if (segment_eq(old_fs, USER_DS)) {
> +		ret = __strncpy_from_unsafe_user(dst, unsafe_addr, count);
> +	} else {
> +		set_fs(USER_DS);
> +		ret = __strncpy_from_unsafe_user(dst, unsafe_addr, count);
> +		set_fs(old_fs);
> +	}
> +	return ret;
>  }

Is that really worth the effort?

Why not keep it simple:

	mm_segment_t old_fs = get_fs();

	set_fs(USER_DS);
	ret = __strncpy...();
	set_fs(old_fd);

	return ret;
?