[RFC,1/2] arm64: write __range_ok() in C

Message ID 20171026090942.7041-2-mark.rutland@arm.com
State New
Headers show
Series
  • arm64: optional paranoid __{get,put}_user checks
Related show

Commit Message

Mark Rutland Oct. 26, 2017, 9:09 a.m.
Currently arm64's __range_ok() is written in assembly for efficiency.

This hides the logic from the compiler, preventing the compiler from
making some optimizations, such as re-ordering instructions or folding
multiple calls to __range_ok().

This patch uses GCC's __builtin_uaddl_overflow() to provide an
equivalent, efficient check, while giving the compiler the visibility it
needs to optimize the check. In testing with v4.14-rc5 using the Linaro
17.05 GCC 6.3.1 toolchain, this has no impact on the kernel Image size,
(but results in a smaller vmlinux).

Signed-off-by: Mark Rutland <mark.rutland@arm.com>

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <labbott@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/uaccess.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.11.0

Comments

Will Deacon Nov. 16, 2017, 3:28 p.m. | #1
On Thu, Oct 26, 2017 at 10:09:41AM +0100, Mark Rutland wrote:
> Currently arm64's __range_ok() is written in assembly for efficiency.

> 

> This hides the logic from the compiler, preventing the compiler from

> making some optimizations, such as re-ordering instructions or folding

> multiple calls to __range_ok().

> 

> This patch uses GCC's __builtin_uaddl_overflow() to provide an

> equivalent, efficient check, while giving the compiler the visibility it

> needs to optimize the check. In testing with v4.14-rc5 using the Linaro

> 17.05 GCC 6.3.1 toolchain, this has no impact on the kernel Image size,

> (but results in a smaller vmlinux).

> 

> Signed-off-by: Mark Rutland <mark.rutland@arm.com>

> Cc: Catalin Marinas <catalin.marinas@arm.com>

> Cc: Kees Cook <keescook@chromium.org>

> Cc: Laura Abbott <labbott@redhat.com>

> Cc: Will Deacon <will.deacon@arm.com>

> ---

>  arch/arm64/include/asm/uaccess.h | 19 +++++++++++--------

>  1 file changed, 11 insertions(+), 8 deletions(-)

> 

> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h

> index fc0f9eb66039..36f84ec92b9d 100644

> --- a/arch/arm64/include/asm/uaccess.h

> +++ b/arch/arm64/include/asm/uaccess.h

> @@ -70,17 +70,20 @@ static inline void set_fs(mm_segment_t fs)

>   *

>   * This needs 65-bit arithmetic.

>   */

> +static bool __range_ok_c(unsigned long addr, unsigned long size)

> +{

> +	unsigned long result;

> +

> +	if (__builtin_uaddl_overflow(addr, size, &result))


I'm not sure if you're planning to revisit this series, but thought I'd
give you a heads up that apparently GCC 4.x doesn't have support for this
builtin, so we'll need to carry the asm at least for that toolchain.

Will
Mark Rutland Nov. 20, 2017, 12:22 p.m. | #2
On Thu, Nov 16, 2017 at 03:28:19PM +0000, Will Deacon wrote:
> On Thu, Oct 26, 2017 at 10:09:41AM +0100, Mark Rutland wrote:

> > +static bool __range_ok_c(unsigned long addr, unsigned long size)

> > +{

> > +	unsigned long result;

> > +

> > +	if (__builtin_uaddl_overflow(addr, size, &result))

> 

> I'm not sure if you're planning to revisit this series, but thought I'd

> give you a heads up that apparently GCC 4.x doesn't have support for this

> builtin, so we'll need to carry the asm at least for that toolchain.


Thanks for the heads-up. I see my Linaro 14.09 GCC 4.9 generates an
out-of-line call to a __builtin_uaddl_overflow helper.

We can avoid the builtin, and write the test in C instead, e.g.

static inline bool __range_ok_c(unsigned long addr, unsigned long size)
{
        unsigned long end = addr + size;

        if (end < addr)
                return false;

        return end <= current_thread_info()->addr_limit;
}

... in my standalone test-case, that generates code that's almost
identical to the builtin, except that the compiler chooses to look at a
different flag.

Thanks,
Mark.

Patch

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index fc0f9eb66039..36f84ec92b9d 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -70,17 +70,20 @@  static inline void set_fs(mm_segment_t fs)
  *
  * This needs 65-bit arithmetic.
  */
+static bool __range_ok_c(unsigned long addr, unsigned long size)
+{
+	unsigned long result;
+
+	if (__builtin_uaddl_overflow(addr, size, &result))
+		return false;
+
+	return result < current_thread_info()->addr_limit;
+}
+
 #define __range_ok(addr, size)						\
 ({									\
-	unsigned long __addr = (unsigned long)(addr);			\
-	unsigned long flag, roksum;					\
 	__chk_user_ptr(addr);						\
-	asm("adds %1, %1, %3; ccmp %1, %4, #2, cc; cset %0, ls"		\
-		: "=&r" (flag), "=&r" (roksum)				\
-		: "1" (__addr), "Ir" (size),				\
-		  "r" (current_thread_info()->addr_limit)		\
-		: "cc");						\
-	flag;								\
+	__range_ok_c((unsigned long)(addr), (unsigned long)(size));	\
 })
 
 /*