* [PATCH v3 0/3] xtensa: fix {get,put}_user() for 64bit values
@ 2019-10-14 21:25 Max Filippov
2019-10-14 21:25 ` [PATCH v3 1/3] " Max Filippov
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Max Filippov @ 2019-10-14 21:25 UTC (permalink / raw)
To: linux-xtensa, Al Viro; +Cc: Chris Zankel, linux-kernel, Max Filippov
Hello,
this series fixes return value, out-of-bound stack access and value
truncation in xtensa implementation of {get,put}_user() for 64bit
values. It also cleans up naming of assembly parameters in
__{get,put}_user_asm and __check_align_{1,2,4}.
Changes v2->v3:
- fix assembly argument constraint for error code
- rearrange result zero-initialization for error paths in __get_user_asm
Changes v1->v2:
- initialize result when access_ok check fails in __get_user_check
- initialize result in __get_user_asm for unaligned access
Al Viro (1):
xtensa: fix {get,put}_user() for 64bit values
Max Filippov (2):
xtensa: clean up assembly arguments in uaccess macros
xtensa: fix type conversion in __get_user_[no]check
arch/xtensa/include/asm/uaccess.h | 94 +++++++++++++++++--------------
1 file changed, 53 insertions(+), 41 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v3 1/3] xtensa: fix {get,put}_user() for 64bit values
2019-10-14 21:25 [PATCH v3 0/3] xtensa: fix {get,put}_user() for 64bit values Max Filippov
@ 2019-10-14 21:25 ` Max Filippov
2019-10-14 21:25 ` [PATCH v3 2/3] xtensa: clean up assembly arguments in uaccess macros Max Filippov
2019-10-14 21:25 ` [PATCH v3 3/3] xtensa: fix type conversion in __get_user_[no]check Max Filippov
2 siblings, 0 replies; 4+ messages in thread
From: Max Filippov @ 2019-10-14 21:25 UTC (permalink / raw)
To: linux-xtensa, Al Viro; +Cc: Chris Zankel, linux-kernel, Max Filippov
From: Al Viro <viro@zeniv.linux.org.uk>
First of all, on short copies __copy_{to,from}_user() return the amount
of bytes left uncopied, *not* -EFAULT. get_user() and put_user() are
expected to return -EFAULT on failure.
Another problem is get_user(v32, (__u64 __user *)p); that should
fetch 64bit value and the assign it to v32, truncating it in process.
Current code, OTOH, reads 8 bytes of data and stores them at the
address of v32, stomping on the 4 bytes that follow v32 itself.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
arch/xtensa/include/asm/uaccess.h | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
index 6792928ba84a..f568c00392ec 100644
--- a/arch/xtensa/include/asm/uaccess.h
+++ b/arch/xtensa/include/asm/uaccess.h
@@ -100,7 +100,7 @@ do { \
case 4: __put_user_asm(x, ptr, retval, 4, "s32i", __cb); break; \
case 8: { \
__typeof__(*ptr) __v64 = x; \
- retval = __copy_to_user(ptr, &__v64, 8); \
+ retval = __copy_to_user(ptr, &__v64, 8) ? -EFAULT : 0; \
break; \
} \
default: __put_user_bad(); \
@@ -198,7 +198,16 @@ do { \
case 1: __get_user_asm(x, ptr, retval, 1, "l8ui", __cb); break;\
case 2: __get_user_asm(x, ptr, retval, 2, "l16ui", __cb); break;\
case 4: __get_user_asm(x, ptr, retval, 4, "l32i", __cb); break;\
- case 8: retval = __copy_from_user(&x, ptr, 8); break; \
+ case 8: { \
+ u64 __x; \
+ if (unlikely(__copy_from_user(&__x, ptr, 8))) { \
+ retval = -EFAULT; \
+ (x) = 0; \
+ } else { \
+ (x) = *(__force __typeof__((ptr)))&__x; \
+ } \
+ break; \
+ } \
default: (x) = __get_user_bad(); \
} \
} while (0)
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/3] xtensa: clean up assembly arguments in uaccess macros
2019-10-14 21:25 [PATCH v3 0/3] xtensa: fix {get,put}_user() for 64bit values Max Filippov
2019-10-14 21:25 ` [PATCH v3 1/3] " Max Filippov
@ 2019-10-14 21:25 ` Max Filippov
2019-10-14 21:25 ` [PATCH v3 3/3] xtensa: fix type conversion in __get_user_[no]check Max Filippov
2 siblings, 0 replies; 4+ messages in thread
From: Max Filippov @ 2019-10-14 21:25 UTC (permalink / raw)
To: linux-xtensa, Al Viro; +Cc: Chris Zankel, linux-kernel, Max Filippov
Numeric assembly arguments are hard to understand and assembly code that
uses them is hard to modify. Use named arguments in __check_align_*,
__get_user_asm and __put_user_asm. Modify macro parameter names so that
they don't affect argument names. Use '+' constraint for the [err]
argument instead of having it as both input and output.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v2->v3:
- fix assembly argument constraint for error code
arch/xtensa/include/asm/uaccess.h | 42 +++++++++++++++----------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
index f568c00392ec..da4d35445063 100644
--- a/arch/xtensa/include/asm/uaccess.h
+++ b/arch/xtensa/include/asm/uaccess.h
@@ -132,14 +132,14 @@ do { \
#define __check_align_1 ""
#define __check_align_2 \
- " _bbci.l %3, 0, 1f \n" \
- " movi %0, %4 \n" \
+ " _bbci.l %[addr], 0, 1f \n" \
+ " movi %[err], %[efault] \n" \
" _j 2f \n"
#define __check_align_4 \
- " _bbsi.l %3, 0, 0f \n" \
- " _bbci.l %3, 1, 1f \n" \
- "0: movi %0, %4 \n" \
+ " _bbsi.l %[addr], 0, 0f \n" \
+ " _bbci.l %[addr], 1, 1f \n" \
+ "0: movi %[err], %[efault] \n" \
" _j 2f \n"
@@ -151,24 +151,24 @@ do { \
* WARNING: If you modify this macro at all, verify that the
* __check_align_* macros still work.
*/
-#define __put_user_asm(x, addr, err, align, insn, cb) \
+#define __put_user_asm(x_, addr_, err_, align, insn, cb)\
__asm__ __volatile__( \
__check_align_##align \
- "1: "insn" %2, %3, 0 \n" \
+ "1: "insn" %[x], %[addr], 0 \n" \
"2: \n" \
" .section .fixup,\"ax\" \n" \
" .align 4 \n" \
" .literal_position \n" \
"5: \n" \
- " movi %1, 2b \n" \
- " movi %0, %4 \n" \
- " jx %1 \n" \
+ " movi %[tmp], 2b \n" \
+ " movi %[err], %[efault] \n" \
+ " jx %[tmp] \n" \
" .previous \n" \
" .section __ex_table,\"a\" \n" \
" .long 1b, 5b \n" \
" .previous" \
- :"=r" (err), "=r" (cb) \
- :"r" ((int)(x)), "r" (addr), "i" (-EFAULT), "0" (err))
+ :[err] "+r"(err_), [tmp] "=r"(cb) \
+ :[x] "r"(x_), [addr] "r"(addr_), [efault] "i"(-EFAULT))
#define __get_user_nocheck(x, ptr, size) \
({ \
@@ -217,25 +217,25 @@ do { \
* WARNING: If you modify this macro at all, verify that the
* __check_align_* macros still work.
*/
-#define __get_user_asm(x, addr, err, align, insn, cb) \
-__asm__ __volatile__( \
+#define __get_user_asm(x_, addr_, err_, align, insn, cb) \
+__asm__ __volatile__( \
__check_align_##align \
- "1: "insn" %2, %3, 0 \n" \
+ "1: "insn" %[x], %[addr], 0 \n" \
"2: \n" \
" .section .fixup,\"ax\" \n" \
" .align 4 \n" \
" .literal_position \n" \
"5: \n" \
- " movi %1, 2b \n" \
- " movi %2, 0 \n" \
- " movi %0, %4 \n" \
- " jx %1 \n" \
+ " movi %[tmp], 2b \n" \
+ " movi %[x], 0 \n" \
+ " movi %[err], %[efault] \n" \
+ " jx %[tmp] \n" \
" .previous \n" \
" .section __ex_table,\"a\" \n" \
" .long 1b, 5b \n" \
" .previous" \
- :"=r" (err), "=r" (cb), "=r" (x) \
- :"r" (addr), "i" (-EFAULT), "0" (err))
+ :[err] "+r"(err_), [tmp] "=r"(cb), [x] "=r"(x_)\
+ :[addr] "r"(addr_), [efault] "i"(-EFAULT))
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 3/3] xtensa: fix type conversion in __get_user_[no]check
2019-10-14 21:25 [PATCH v3 0/3] xtensa: fix {get,put}_user() for 64bit values Max Filippov
2019-10-14 21:25 ` [PATCH v3 1/3] " Max Filippov
2019-10-14 21:25 ` [PATCH v3 2/3] xtensa: clean up assembly arguments in uaccess macros Max Filippov
@ 2019-10-14 21:25 ` Max Filippov
2 siblings, 0 replies; 4+ messages in thread
From: Max Filippov @ 2019-10-14 21:25 UTC (permalink / raw)
To: linux-xtensa, Al Viro; +Cc: Chris Zankel, linux-kernel, Max Filippov
__get_user_[no]check uses temporary buffer of type long to store result
of __get_user_size and do sign extension on it when necessary. This
doesn't work correctly for 64-bit data. Fix it by moving temporary
buffer/sign extension logic to __get_user_asm.
Don't do assignment of __get_user_bad result to (x) as it may not always
be integer-compatible now and issue warning even when it's going to be
optimized. Instead do (x) = 0; and call __get_user_bad separately.
Zero initialize __x in __get_user_asm and use '+' constraint for its
assembly argument, so that its value is preserved in error cases. This
may add at most 1 cycle to the fast path, but saves an instruction and
two padding bytes in the fixup section for each use of this macro and
works for both misaligned store and store exception.
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
Changes v2->v3:
- rearrange result zero-initialization for error paths in __get_user_asm
Changes v1->v2:
- initialize result when access_ok check fails in __get_user_check
arch/xtensa/include/asm/uaccess.h | 55 ++++++++++++++++---------------
1 file changed, 29 insertions(+), 26 deletions(-)
diff --git a/arch/xtensa/include/asm/uaccess.h b/arch/xtensa/include/asm/uaccess.h
index da4d35445063..3f80386f1883 100644
--- a/arch/xtensa/include/asm/uaccess.h
+++ b/arch/xtensa/include/asm/uaccess.h
@@ -172,19 +172,19 @@ __asm__ __volatile__( \
#define __get_user_nocheck(x, ptr, size) \
({ \
- long __gu_err, __gu_val; \
- __get_user_size(__gu_val, (ptr), (size), __gu_err); \
- (x) = (__force __typeof__(*(ptr)))__gu_val; \
+ long __gu_err; \
+ __get_user_size((x), (ptr), (size), __gu_err); \
__gu_err; \
})
#define __get_user_check(x, ptr, size) \
({ \
- long __gu_err = -EFAULT, __gu_val = 0; \
+ long __gu_err = -EFAULT; \
const __typeof__(*(ptr)) *__gu_addr = (ptr); \
- if (access_ok(__gu_addr, size)) \
- __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
- (x) = (__force __typeof__(*(ptr)))__gu_val; \
+ if (access_ok(__gu_addr, size)) \
+ __get_user_size((x), __gu_addr, (size), __gu_err); \
+ else \
+ (x) = 0; \
__gu_err; \
})
@@ -208,7 +208,7 @@ do { \
} \
break; \
} \
- default: (x) = __get_user_bad(); \
+ default: (x) = 0; __get_user_bad(); \
} \
} while (0)
@@ -218,24 +218,27 @@ do { \
* __check_align_* macros still work.
*/
#define __get_user_asm(x_, addr_, err_, align, insn, cb) \
-__asm__ __volatile__( \
- __check_align_##align \
- "1: "insn" %[x], %[addr], 0 \n" \
- "2: \n" \
- " .section .fixup,\"ax\" \n" \
- " .align 4 \n" \
- " .literal_position \n" \
- "5: \n" \
- " movi %[tmp], 2b \n" \
- " movi %[x], 0 \n" \
- " movi %[err], %[efault] \n" \
- " jx %[tmp] \n" \
- " .previous \n" \
- " .section __ex_table,\"a\" \n" \
- " .long 1b, 5b \n" \
- " .previous" \
- :[err] "+r"(err_), [tmp] "=r"(cb), [x] "=r"(x_)\
- :[addr] "r"(addr_), [efault] "i"(-EFAULT))
+do { \
+ u32 __x = 0; \
+ __asm__ __volatile__( \
+ __check_align_##align \
+ "1: "insn" %[x], %[addr], 0 \n" \
+ "2: \n" \
+ " .section .fixup,\"ax\" \n" \
+ " .align 4 \n" \
+ " .literal_position \n" \
+ "5: \n" \
+ " movi %[tmp], 2b \n" \
+ " movi %[err], %[efault] \n" \
+ " jx %[tmp] \n" \
+ " .previous \n" \
+ " .section __ex_table,\"a\" \n" \
+ " .long 1b, 5b \n" \
+ " .previous" \
+ :[err] "+r"(err_), [tmp] "=r"(cb), [x] "+r"(__x) \
+ :[addr] "r"(addr_), [efault] "i"(-EFAULT)); \
+ (x_) = (__force __typeof__(*(addr_)))__x; \
+} while (0)
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-14 21:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-14 21:25 [PATCH v3 0/3] xtensa: fix {get,put}_user() for 64bit values Max Filippov
2019-10-14 21:25 ` [PATCH v3 1/3] " Max Filippov
2019-10-14 21:25 ` [PATCH v3 2/3] xtensa: clean up assembly arguments in uaccess macros Max Filippov
2019-10-14 21:25 ` [PATCH v3 3/3] xtensa: fix type conversion in __get_user_[no]check Max Filippov
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).