| Message ID | 20171026090942.7041-2-mark.rutland@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: optional paranoid __{get,put}_user checks | expand |
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
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.
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)); \ }) /*
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