* [PATCH 0/3] metag/usercopy: Support RAW_COPY_USER @ 2017-04-04 14:46 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag, Al Viro; +Cc: James Hogan, stable These patches, based on viro-vfs/uaccess.metag, add support for RAW_COPY_USER to the metag architecture. Al: Did you want these to go via your uaccess.metag branch? If so I'm happy to create a topic branch with the fixes (patches 1-2, marked for stable) for you to pull, so I can get them upstream for 4.11. - Patch 1 drops some unused macros so they don't need fixing in patch 2. - Patch 2 moves zeroing out to copy_from_user, since __copy_from_user shouldn't cause zeroing, and the existing fixup code didn't properly zero the access that first failed anyway. This is needed for patch 3. - Patch 3 switches metag to using RAW_COPY_USER, which is fairly trivial now that patch 2 moved the zeroing out to copy_from_user. I have a bunch of other usercopy fixes for metag, but it won't hurt for them to go in separately to these patches. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org James Hogan (3): metag/usercopy: Drop unused macros metag/usercopy: Zero rest of buffer from copy_from_user metag/usercopy: Switch to RAW_COPY_USER arch/metag/Kconfig | 1 +- arch/metag/include/asm/uaccess.h | 34 +------ arch/metag/lib/usercopy.c | 171 +++----------------------------- 3 files changed, 26 insertions(+), 180 deletions(-) -- git-series 0.8.10 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 0/3] metag/usercopy: Support RAW_COPY_USER @ 2017-04-04 14:46 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag, Al Viro; +Cc: James Hogan, stable These patches, based on viro-vfs/uaccess.metag, add support for RAW_COPY_USER to the metag architecture. Al: Did you want these to go via your uaccess.metag branch? If so I'm happy to create a topic branch with the fixes (patches 1-2, marked for stable) for you to pull, so I can get them upstream for 4.11. - Patch 1 drops some unused macros so they don't need fixing in patch 2. - Patch 2 moves zeroing out to copy_from_user, since __copy_from_user shouldn't cause zeroing, and the existing fixup code didn't properly zero the access that first failed anyway. This is needed for patch 3. - Patch 3 switches metag to using RAW_COPY_USER, which is fairly trivial now that patch 2 moved the zeroing out to copy_from_user. I have a bunch of other usercopy fixes for metag, but it won't hurt for them to go in separately to these patches. Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org James Hogan (3): metag/usercopy: Drop unused macros metag/usercopy: Zero rest of buffer from copy_from_user metag/usercopy: Switch to RAW_COPY_USER arch/metag/Kconfig | 1 +- arch/metag/include/asm/uaccess.h | 34 +------ arch/metag/lib/usercopy.c | 171 +++----------------------------- 3 files changed, 26 insertions(+), 180 deletions(-) -- git-series 0.8.10 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] metag/usercopy: Drop unused macros [not found] ` <cover.67b8034e25703fa0c68295f1ac1d7abe4bd1fac9.1491316836.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2017-04-04 14:46 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag, Al Viro; +Cc: James Hogan, stable Metag's lib/usercopy.c has a bunch of copy_from_user macros for larger copies between 5 and 16 bytes which are completely unused. Before fixing zeroing lets drop these macros so there is less to fix. Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org --- arch/metag/lib/usercopy.c | 113 +--------------------------------------- 1 file changed, 0 insertions(+), 113 deletions(-) diff --git a/arch/metag/lib/usercopy.c b/arch/metag/lib/usercopy.c index b3ebfe9c8e88..b4eb1f17069f 100644 --- a/arch/metag/lib/usercopy.c +++ b/arch/metag/lib/usercopy.c @@ -651,119 +651,6 @@ EXPORT_SYMBOL(__copy_user); #define __asm_copy_from_user_4(to, from, ret) \ __asm_copy_from_user_4x_cont(to, from, ret, "", "", "") -#define __asm_copy_from_user_5(to, from, ret) \ - __asm_copy_from_user_4x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "4: SETB [%0++],D1Ar1\n", \ - "5: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 4b,5b\n") - -#define __asm_copy_from_user_6x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_4x_cont(to, from, ret, \ - " GETW D1Ar1,[%1++]\n" \ - "4: SETW [%0++],D1Ar1\n" COPY, \ - "5: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ - " .long 4b,5b\n" TENTRY) - -#define __asm_copy_from_user_6(to, from, ret) \ - __asm_copy_from_user_6x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_7(to, from, ret) \ - __asm_copy_from_user_6x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "6: SETB [%0++],D1Ar1\n", \ - "7: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 6b,7b\n") - -#define __asm_copy_from_user_8x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_4x_cont(to, from, ret, \ - " GETD D1Ar1,[%1++]\n" \ - "4: SETD [%0++],D1Ar1\n" COPY, \ - "5: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ - " .long 4b,5b\n" TENTRY) - -#define __asm_copy_from_user_8(to, from, ret) \ - __asm_copy_from_user_8x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_9(to, from, ret) \ - __asm_copy_from_user_8x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "6: SETB [%0++],D1Ar1\n", \ - "7: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 6b,7b\n") - -#define __asm_copy_from_user_10x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_8x_cont(to, from, ret, \ - " GETW D1Ar1,[%1++]\n" \ - "6: SETW [%0++],D1Ar1\n" COPY, \ - "7: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ - " .long 6b,7b\n" TENTRY) - -#define __asm_copy_from_user_10(to, from, ret) \ - __asm_copy_from_user_10x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_11(to, from, ret) \ - __asm_copy_from_user_10x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "8: SETB [%0++],D1Ar1\n", \ - "9: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 8b,9b\n") - -#define __asm_copy_from_user_12x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_8x_cont(to, from, ret, \ - " GETD D1Ar1,[%1++]\n" \ - "6: SETD [%0++],D1Ar1\n" COPY, \ - "7: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ - " .long 6b,7b\n" TENTRY) - -#define __asm_copy_from_user_12(to, from, ret) \ - __asm_copy_from_user_12x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_13(to, from, ret) \ - __asm_copy_from_user_12x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "8: SETB [%0++],D1Ar1\n", \ - "9: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 8b,9b\n") - -#define __asm_copy_from_user_14x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_12x_cont(to, from, ret, \ - " GETW D1Ar1,[%1++]\n" \ - "8: SETW [%0++],D1Ar1\n" COPY, \ - "9: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ - " .long 8b,9b\n" TENTRY) - -#define __asm_copy_from_user_14(to, from, ret) \ - __asm_copy_from_user_14x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_15(to, from, ret) \ - __asm_copy_from_user_14x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "10: SETB [%0++],D1Ar1\n", \ - "11: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 10b,11b\n") - -#define __asm_copy_from_user_16x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_12x_cont(to, from, ret, \ - " GETD D1Ar1,[%1++]\n" \ - "8: SETD [%0++],D1Ar1\n" COPY, \ - "9: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ - " .long 8b,9b\n" TENTRY) - -#define __asm_copy_from_user_16(to, from, ret) \ - __asm_copy_from_user_16x_cont(to, from, ret, "", "", "") #define __asm_copy_from_user_8x64(to, from, ret) \ asm volatile ( \ -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 1/3] metag/usercopy: Drop unused macros @ 2017-04-04 14:46 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag-u79uwXL29TY76Z2rM5mHXA, Al Viro Cc: James Hogan, stable-u79uwXL29TY76Z2rM5mHXA Metag's lib/usercopy.c has a bunch of copy_from_user macros for larger copies between 5 and 16 bytes which are completely unused. Before fixing zeroing lets drop these macros so there is less to fix. Signed-off-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org --- arch/metag/lib/usercopy.c | 113 +--------------------------------------- 1 file changed, 0 insertions(+), 113 deletions(-) diff --git a/arch/metag/lib/usercopy.c b/arch/metag/lib/usercopy.c index b3ebfe9c8e88..b4eb1f17069f 100644 --- a/arch/metag/lib/usercopy.c +++ b/arch/metag/lib/usercopy.c @@ -651,119 +651,6 @@ EXPORT_SYMBOL(__copy_user); #define __asm_copy_from_user_4(to, from, ret) \ __asm_copy_from_user_4x_cont(to, from, ret, "", "", "") -#define __asm_copy_from_user_5(to, from, ret) \ - __asm_copy_from_user_4x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "4: SETB [%0++],D1Ar1\n", \ - "5: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 4b,5b\n") - -#define __asm_copy_from_user_6x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_4x_cont(to, from, ret, \ - " GETW D1Ar1,[%1++]\n" \ - "4: SETW [%0++],D1Ar1\n" COPY, \ - "5: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ - " .long 4b,5b\n" TENTRY) - -#define __asm_copy_from_user_6(to, from, ret) \ - __asm_copy_from_user_6x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_7(to, from, ret) \ - __asm_copy_from_user_6x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "6: SETB [%0++],D1Ar1\n", \ - "7: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 6b,7b\n") - -#define __asm_copy_from_user_8x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_4x_cont(to, from, ret, \ - " GETD D1Ar1,[%1++]\n" \ - "4: SETD [%0++],D1Ar1\n" COPY, \ - "5: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ - " .long 4b,5b\n" TENTRY) - -#define __asm_copy_from_user_8(to, from, ret) \ - __asm_copy_from_user_8x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_9(to, from, ret) \ - __asm_copy_from_user_8x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "6: SETB [%0++],D1Ar1\n", \ - "7: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 6b,7b\n") - -#define __asm_copy_from_user_10x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_8x_cont(to, from, ret, \ - " GETW D1Ar1,[%1++]\n" \ - "6: SETW [%0++],D1Ar1\n" COPY, \ - "7: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ - " .long 6b,7b\n" TENTRY) - -#define __asm_copy_from_user_10(to, from, ret) \ - __asm_copy_from_user_10x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_11(to, from, ret) \ - __asm_copy_from_user_10x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "8: SETB [%0++],D1Ar1\n", \ - "9: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 8b,9b\n") - -#define __asm_copy_from_user_12x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_8x_cont(to, from, ret, \ - " GETD D1Ar1,[%1++]\n" \ - "6: SETD [%0++],D1Ar1\n" COPY, \ - "7: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ - " .long 6b,7b\n" TENTRY) - -#define __asm_copy_from_user_12(to, from, ret) \ - __asm_copy_from_user_12x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_13(to, from, ret) \ - __asm_copy_from_user_12x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "8: SETB [%0++],D1Ar1\n", \ - "9: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 8b,9b\n") - -#define __asm_copy_from_user_14x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_12x_cont(to, from, ret, \ - " GETW D1Ar1,[%1++]\n" \ - "8: SETW [%0++],D1Ar1\n" COPY, \ - "9: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ - " .long 8b,9b\n" TENTRY) - -#define __asm_copy_from_user_14(to, from, ret) \ - __asm_copy_from_user_14x_cont(to, from, ret, "", "", "") - -#define __asm_copy_from_user_15(to, from, ret) \ - __asm_copy_from_user_14x_cont(to, from, ret, \ - " GETB D1Ar1,[%1++]\n" \ - "10: SETB [%0++],D1Ar1\n", \ - "11: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ - " .long 10b,11b\n") - -#define __asm_copy_from_user_16x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ - __asm_copy_from_user_12x_cont(to, from, ret, \ - " GETD D1Ar1,[%1++]\n" \ - "8: SETD [%0++],D1Ar1\n" COPY, \ - "9: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ - " .long 8b,9b\n" TENTRY) - -#define __asm_copy_from_user_16(to, from, ret) \ - __asm_copy_from_user_16x_cont(to, from, ret, "", "", "") #define __asm_copy_from_user_8x64(to, from, ret) \ asm volatile ( \ -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user 2017-04-04 14:46 ` James Hogan @ 2017-04-04 14:46 ` James Hogan -1 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag, Al Viro; +Cc: James Hogan, stable Currently we try to zero the destination for a failed read from userland in fixup code in the usercopy.c macros. The rest of the destination buffer is then zeroed from __copy_user_zeroing(), which is used for both copy_from_user() and __copy_from_user(). Unfortunately we fail to zero in the fixup code as D1Ar1 is set to 0 before the fixup code entry labels, and __copy_from_user() shouldn't even be zeroing the rest of the buffer. Move the zeroing out into copy_from_user() and rename __copy_user_zeroing() to raw_copy_from_user() since it no longer does any zeroing. This also conveniently matches the name needed for RAW_COPY_USER support in a later patch. Reported-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org --- arch/metag/include/asm/uaccess.h | 15 +++++----- arch/metag/lib/usercopy.c | 52 +++++++++++---------------------- 2 files changed, 26 insertions(+), 41 deletions(-) diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h index 2613d91bde1a..61805d9f9375 100644 --- a/arch/metag/include/asm/uaccess.h +++ b/arch/metag/include/asm/uaccess.h @@ -172,20 +172,21 @@ extern long __must_check strnlen_user(const char __user *src, long count); #define strlen_user(str) strnlen_user(str, 32767) -extern unsigned long __must_check __copy_user_zeroing(void *to, - const void __user *from, - unsigned long n); +extern unsigned long raw_copy_from_user(void *to, const void __user *from, + unsigned long n); static inline unsigned long copy_from_user(void *to, const void __user *from, unsigned long n) { + unsigned long res = n; if (likely(access_ok(VERIFY_READ, from, n))) - return __copy_user_zeroing(to, from, n); - memset(to, 0, n); - return n; + res = raw_copy_from_user(to, from, n); + if (unlikely(res)) + memset(to + (n - res), 0, res); + return res; } -#define __copy_from_user(to, from, n) __copy_user_zeroing(to, from, n) +#define __copy_from_user(to, from, n) raw_copy_from_user(to, from, n) #define __copy_from_user_inatomic __copy_from_user extern unsigned long __must_check __copy_user(void __user *to, diff --git a/arch/metag/lib/usercopy.c b/arch/metag/lib/usercopy.c index b4eb1f17069f..bb1f7d60fcc0 100644 --- a/arch/metag/lib/usercopy.c +++ b/arch/metag/lib/usercopy.c @@ -29,7 +29,6 @@ COPY \ "1:\n" \ " .section .fixup,\"ax\"\n" \ - " MOV D1Ar1,#0\n" \ FIXUP \ " MOVT D1Ar1,#HI(1b)\n" \ " JUMP D1Ar1,#LO(1b)\n" \ @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user); " GETB D1Ar1,[%1++]\n" \ "2: SETB [%0++],D1Ar1\n", \ "3: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ + " ADD %0,%0,#1\n", \ " .long 2b,3b\n") #define __asm_copy_from_user_2x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ @@ -626,7 +625,7 @@ EXPORT_SYMBOL(__copy_user); " GETW D1Ar1,[%1++]\n" \ "2: SETW [%0++],D1Ar1\n" COPY, \ "3: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ + " ADD %0,%0,#2\n" FIXUP, \ " .long 2b,3b\n" TENTRY) #define __asm_copy_from_user_2(to, from, ret) \ @@ -637,7 +636,7 @@ EXPORT_SYMBOL(__copy_user); " GETB D1Ar1,[%1++]\n" \ "4: SETB [%0++],D1Ar1\n", \ "5: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ + " ADD %0,%0,#1\n", \ " .long 4b,5b\n") #define __asm_copy_from_user_4x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ @@ -645,23 +644,20 @@ EXPORT_SYMBOL(__copy_user); " GETD D1Ar1,[%1++]\n" \ "2: SETD [%0++],D1Ar1\n" COPY, \ "3: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ + " ADD %0,%0,#4\n" FIXUP, \ " .long 2b,3b\n" TENTRY) #define __asm_copy_from_user_4(to, from, ret) \ __asm_copy_from_user_4x_cont(to, from, ret, "", "", "") - #define __asm_copy_from_user_8x64(to, from, ret) \ asm volatile ( \ " GETL D0Ar2,D1Ar1,[%1++]\n" \ "2: SETL [%0++],D0Ar2,D1Ar1\n" \ "1:\n" \ " .section .fixup,\"ax\"\n" \ - " MOV D1Ar1,#0\n" \ - " MOV D0Ar2,#0\n" \ "3: ADD %2,%2,#8\n" \ - " SETL [%0++],D0Ar2,D1Ar1\n" \ + " ADD %0,%0,#8\n" \ " MOVT D0Ar2,#HI(1b)\n" \ " JUMP D0Ar2,#LO(1b)\n" \ " .previous\n" \ @@ -701,11 +697,12 @@ EXPORT_SYMBOL(__copy_user); "SUB %1, %1, #4\n") -/* Copy from user to kernel, zeroing the bytes that were inaccessible in - userland. The return-value is the number of bytes that were - inaccessible. */ -unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, - unsigned long n) +/* + * Copy from user to kernel. The return-value is the number of bytes that were + * inaccessible. + */ +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, + unsigned long n) { register char *dst asm ("A0.2") = pdst; register const char __user *src asm ("A1.2") = psrc; @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_1(dst, src, retn); n--; if (retn) - goto copy_exception_bytes; + return retn + n; } } if (((unsigned long) src & 2) && n >= 2) { @@ -737,7 +734,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_2(dst, src, retn); n -= 2; if (retn) - goto copy_exception_bytes; + return retn + n; } } @@ -745,7 +742,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, because if both adjustments were done, either both or neither reference had an exception. */ if (retn != 0) - goto copy_exception_bytes; + return retn + n; #ifdef USE_RAPF /* 64 bit copy loop */ @@ -759,7 +756,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_8x64(dst, src, retn); n -= 8; if (retn) - goto copy_exception_bytes; + return retn + n; } } @@ -775,7 +772,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_8x64(dst, src, retn); n -= 8; if (retn) - goto copy_exception_bytes; + return retn + n; } } #endif @@ -785,7 +782,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, n -= 4; if (retn) - goto copy_exception_bytes; + return retn + n; } /* If we get here, there were no memory read faults. */ @@ -811,21 +808,8 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, /* If we get here, retn correctly reflects the number of failing bytes. */ return retn; - - copy_exception_bytes: - /* We already have "retn" bytes cleared, and need to clear the - remaining "n" bytes. A non-optimized simple byte-for-byte in-line - memset is preferred here, since this isn't speed-critical code and - we'd rather have this a leaf-function than calling memset. */ - { - char *endp; - for (endp = dst + n; dst < endp; dst++) - *dst = 0; - } - - return retn + n; } -EXPORT_SYMBOL(__copy_user_zeroing); +EXPORT_SYMBOL(raw_copy_from_user); #define __asm_clear_8x64(to, ret) \ asm volatile ( \ -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user @ 2017-04-04 14:46 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag, Al Viro; +Cc: James Hogan, stable Currently we try to zero the destination for a failed read from userland in fixup code in the usercopy.c macros. The rest of the destination buffer is then zeroed from __copy_user_zeroing(), which is used for both copy_from_user() and __copy_from_user(). Unfortunately we fail to zero in the fixup code as D1Ar1 is set to 0 before the fixup code entry labels, and __copy_from_user() shouldn't even be zeroing the rest of the buffer. Move the zeroing out into copy_from_user() and rename __copy_user_zeroing() to raw_copy_from_user() since it no longer does any zeroing. This also conveniently matches the name needed for RAW_COPY_USER support in a later patch. Reported-by: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: linux-metag@vger.kernel.org Cc: stable@vger.kernel.org --- arch/metag/include/asm/uaccess.h | 15 +++++----- arch/metag/lib/usercopy.c | 52 +++++++++++---------------------- 2 files changed, 26 insertions(+), 41 deletions(-) diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h index 2613d91bde1a..61805d9f9375 100644 --- a/arch/metag/include/asm/uaccess.h +++ b/arch/metag/include/asm/uaccess.h @@ -172,20 +172,21 @@ extern long __must_check strnlen_user(const char __user *src, long count); #define strlen_user(str) strnlen_user(str, 32767) -extern unsigned long __must_check __copy_user_zeroing(void *to, - const void __user *from, - unsigned long n); +extern unsigned long raw_copy_from_user(void *to, const void __user *from, + unsigned long n); static inline unsigned long copy_from_user(void *to, const void __user *from, unsigned long n) { + unsigned long res = n; if (likely(access_ok(VERIFY_READ, from, n))) - return __copy_user_zeroing(to, from, n); - memset(to, 0, n); - return n; + res = raw_copy_from_user(to, from, n); + if (unlikely(res)) + memset(to + (n - res), 0, res); + return res; } -#define __copy_from_user(to, from, n) __copy_user_zeroing(to, from, n) +#define __copy_from_user(to, from, n) raw_copy_from_user(to, from, n) #define __copy_from_user_inatomic __copy_from_user extern unsigned long __must_check __copy_user(void __user *to, diff --git a/arch/metag/lib/usercopy.c b/arch/metag/lib/usercopy.c index b4eb1f17069f..bb1f7d60fcc0 100644 --- a/arch/metag/lib/usercopy.c +++ b/arch/metag/lib/usercopy.c @@ -29,7 +29,6 @@ COPY \ "1:\n" \ " .section .fixup,\"ax\"\n" \ - " MOV D1Ar1,#0\n" \ FIXUP \ " MOVT D1Ar1,#HI(1b)\n" \ " JUMP D1Ar1,#LO(1b)\n" \ @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user); " GETB D1Ar1,[%1++]\n" \ "2: SETB [%0++],D1Ar1\n", \ "3: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ + " ADD %0,%0,#1\n", \ " .long 2b,3b\n") #define __asm_copy_from_user_2x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ @@ -626,7 +625,7 @@ EXPORT_SYMBOL(__copy_user); " GETW D1Ar1,[%1++]\n" \ "2: SETW [%0++],D1Ar1\n" COPY, \ "3: ADD %2,%2,#2\n" \ - " SETW [%0++],D1Ar1\n" FIXUP, \ + " ADD %0,%0,#2\n" FIXUP, \ " .long 2b,3b\n" TENTRY) #define __asm_copy_from_user_2(to, from, ret) \ @@ -637,7 +636,7 @@ EXPORT_SYMBOL(__copy_user); " GETB D1Ar1,[%1++]\n" \ "4: SETB [%0++],D1Ar1\n", \ "5: ADD %2,%2,#1\n" \ - " SETB [%0++],D1Ar1\n", \ + " ADD %0,%0,#1\n", \ " .long 4b,5b\n") #define __asm_copy_from_user_4x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ @@ -645,23 +644,20 @@ EXPORT_SYMBOL(__copy_user); " GETD D1Ar1,[%1++]\n" \ "2: SETD [%0++],D1Ar1\n" COPY, \ "3: ADD %2,%2,#4\n" \ - " SETD [%0++],D1Ar1\n" FIXUP, \ + " ADD %0,%0,#4\n" FIXUP, \ " .long 2b,3b\n" TENTRY) #define __asm_copy_from_user_4(to, from, ret) \ __asm_copy_from_user_4x_cont(to, from, ret, "", "", "") - #define __asm_copy_from_user_8x64(to, from, ret) \ asm volatile ( \ " GETL D0Ar2,D1Ar1,[%1++]\n" \ "2: SETL [%0++],D0Ar2,D1Ar1\n" \ "1:\n" \ " .section .fixup,\"ax\"\n" \ - " MOV D1Ar1,#0\n" \ - " MOV D0Ar2,#0\n" \ "3: ADD %2,%2,#8\n" \ - " SETL [%0++],D0Ar2,D1Ar1\n" \ + " ADD %0,%0,#8\n" \ " MOVT D0Ar2,#HI(1b)\n" \ " JUMP D0Ar2,#LO(1b)\n" \ " .previous\n" \ @@ -701,11 +697,12 @@ EXPORT_SYMBOL(__copy_user); "SUB %1, %1, #4\n") -/* Copy from user to kernel, zeroing the bytes that were inaccessible in - userland. The return-value is the number of bytes that were - inaccessible. */ -unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, - unsigned long n) +/* + * Copy from user to kernel. The return-value is the number of bytes that were + * inaccessible. + */ +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, + unsigned long n) { register char *dst asm ("A0.2") = pdst; register const char __user *src asm ("A1.2") = psrc; @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_1(dst, src, retn); n--; if (retn) - goto copy_exception_bytes; + return retn + n; } } if (((unsigned long) src & 2) && n >= 2) { @@ -737,7 +734,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_2(dst, src, retn); n -= 2; if (retn) - goto copy_exception_bytes; + return retn + n; } } @@ -745,7 +742,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, because if both adjustments were done, either both or neither reference had an exception. */ if (retn != 0) - goto copy_exception_bytes; + return retn + n; #ifdef USE_RAPF /* 64 bit copy loop */ @@ -759,7 +756,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_8x64(dst, src, retn); n -= 8; if (retn) - goto copy_exception_bytes; + return retn + n; } } @@ -775,7 +772,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, __asm_copy_from_user_8x64(dst, src, retn); n -= 8; if (retn) - goto copy_exception_bytes; + return retn + n; } } #endif @@ -785,7 +782,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, n -= 4; if (retn) - goto copy_exception_bytes; + return retn + n; } /* If we get here, there were no memory read faults. */ @@ -811,21 +808,8 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, /* If we get here, retn correctly reflects the number of failing bytes. */ return retn; - - copy_exception_bytes: - /* We already have "retn" bytes cleared, and need to clear the - remaining "n" bytes. A non-optimized simple byte-for-byte in-line - memset is preferred here, since this isn't speed-critical code and - we'd rather have this a leaf-function than calling memset. */ - { - char *endp; - for (endp = dst + n; dst < endp; dst++) - *dst = 0; - } - - return retn + n; } -EXPORT_SYMBOL(__copy_user_zeroing); +EXPORT_SYMBOL(raw_copy_from_user); #define __asm_clear_8x64(to, ret) \ asm volatile ( \ -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user @ 2017-04-04 16:31 ` Al Viro 0 siblings, 0 replies; 11+ messages in thread From: Al Viro @ 2017-04-04 16:31 UTC (permalink / raw) To: James Hogan; +Cc: linux-metag, stable On Tue, Apr 04, 2017 at 03:46:51PM +0100, James Hogan wrote: > @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user); > " GETB D1Ar1,[%1++]\n" \ > "2: SETB [%0++],D1Ar1\n", \ > "3: ADD %2,%2,#1\n" \ > - " SETB [%0++],D1Ar1\n", \ > + " ADD %0,%0,#1\n", \ Why bother incrementing dst here? > " .long 2b,3b\n") > > #define __asm_copy_from_user_2x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ > @@ -626,7 +625,7 @@ EXPORT_SYMBOL(__copy_user); > " GETW D1Ar1,[%1++]\n" \ > "2: SETW [%0++],D1Ar1\n" COPY, \ > "3: ADD %2,%2,#2\n" \ > - " SETW [%0++],D1Ar1\n" FIXUP, \ > + " ADD %0,%0,#2\n" FIXUP, \ > " .long 2b,3b\n" TENTRY) > > #define __asm_copy_from_user_2(to, from, ret) \ > @@ -637,7 +636,7 @@ EXPORT_SYMBOL(__copy_user); > " GETB D1Ar1,[%1++]\n" \ > "4: SETB [%0++],D1Ar1\n", \ > "5: ADD %2,%2,#1\n" \ > - " SETB [%0++],D1Ar1\n", \ > + " ADD %0,%0,#1\n", \ > " .long 4b,5b\n") > > #define __asm_copy_from_user_4x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ > @@ -645,23 +644,20 @@ EXPORT_SYMBOL(__copy_user); > " GETD D1Ar1,[%1++]\n" \ > "2: SETD [%0++],D1Ar1\n" COPY, \ > "3: ADD %2,%2,#4\n" \ > - " SETD [%0++],D1Ar1\n" FIXUP, \ > + " ADD %0,%0,#4\n" FIXUP, \ > " .long 2b,3b\n" TENTRY) > > #define __asm_copy_from_user_8x64(to, from, ret) \ > asm volatile ( \ > " GETL D0Ar2,D1Ar1,[%1++]\n" \ > "2: SETL [%0++],D0Ar2,D1Ar1\n" \ > "1:\n" \ > " .section .fixup,\"ax\"\n" \ > - " MOV D1Ar1,#0\n" \ > - " MOV D0Ar2,#0\n" \ > "3: ADD %2,%2,#8\n" \ > - " SETL [%0++],D0Ar2,D1Ar1\n" \ > + " ADD %0,%0,#8\n" \ > " MOVT D0Ar2,#HI(1b)\n" \ > " JUMP D0Ar2,#LO(1b)\n" \ > " .previous\n" \ Ditto for all of those, actually. > +/* > + * Copy from user to kernel. The return-value is the number of bytes that were > + * inaccessible. > + */ > +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, > + unsigned long n) > { > register char *dst asm ("A0.2") = pdst; > register const char __user *src asm ("A1.2") = psrc; > @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, > __asm_copy_from_user_1(dst, src, retn); > n--; > if (retn) > - goto copy_exception_bytes; > + return retn + n; > } > } Umm... What happens if psrc points to the last byte of an unmapped page, with psrc + 1 pointing to valid data? AFAICS, you'll get GETB fail, have retn increased to 1, n decremented by 1, then, assuming you end up in that byte-copy loop, have GETB at src + 1 *succeed*, the value fetched fed to SETB at dst + 1, n decremented again, non-zero retn finally spotted and n + retn returned, reporting that you've copied a single byte. Which you certainly have, but it's pdst[1] = psrc[1], not pdst[0] = psrc[0]. IOW, you need to check retn after all __asm_copy_from_user_{1,2} in the beginning. As for topic branch for #1 and #2 - sure, perfectly fine by me. Just give me the branch name and I'll pull. But I think the scenario above needs to be dealt with... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user @ 2017-04-04 16:31 ` Al Viro 0 siblings, 0 replies; 11+ messages in thread From: Al Viro @ 2017-04-04 16:31 UTC (permalink / raw) To: James Hogan Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA On Tue, Apr 04, 2017 at 03:46:51PM +0100, James Hogan wrote: > @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user); > " GETB D1Ar1,[%1++]\n" \ > "2: SETB [%0++],D1Ar1\n", \ > "3: ADD %2,%2,#1\n" \ > - " SETB [%0++],D1Ar1\n", \ > + " ADD %0,%0,#1\n", \ Why bother incrementing dst here? > " .long 2b,3b\n") > > #define __asm_copy_from_user_2x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ > @@ -626,7 +625,7 @@ EXPORT_SYMBOL(__copy_user); > " GETW D1Ar1,[%1++]\n" \ > "2: SETW [%0++],D1Ar1\n" COPY, \ > "3: ADD %2,%2,#2\n" \ > - " SETW [%0++],D1Ar1\n" FIXUP, \ > + " ADD %0,%0,#2\n" FIXUP, \ > " .long 2b,3b\n" TENTRY) > > #define __asm_copy_from_user_2(to, from, ret) \ > @@ -637,7 +636,7 @@ EXPORT_SYMBOL(__copy_user); > " GETB D1Ar1,[%1++]\n" \ > "4: SETB [%0++],D1Ar1\n", \ > "5: ADD %2,%2,#1\n" \ > - " SETB [%0++],D1Ar1\n", \ > + " ADD %0,%0,#1\n", \ > " .long 4b,5b\n") > > #define __asm_copy_from_user_4x_cont(to, from, ret, COPY, FIXUP, TENTRY) \ > @@ -645,23 +644,20 @@ EXPORT_SYMBOL(__copy_user); > " GETD D1Ar1,[%1++]\n" \ > "2: SETD [%0++],D1Ar1\n" COPY, \ > "3: ADD %2,%2,#4\n" \ > - " SETD [%0++],D1Ar1\n" FIXUP, \ > + " ADD %0,%0,#4\n" FIXUP, \ > " .long 2b,3b\n" TENTRY) > > #define __asm_copy_from_user_8x64(to, from, ret) \ > asm volatile ( \ > " GETL D0Ar2,D1Ar1,[%1++]\n" \ > "2: SETL [%0++],D0Ar2,D1Ar1\n" \ > "1:\n" \ > " .section .fixup,\"ax\"\n" \ > - " MOV D1Ar1,#0\n" \ > - " MOV D0Ar2,#0\n" \ > "3: ADD %2,%2,#8\n" \ > - " SETL [%0++],D0Ar2,D1Ar1\n" \ > + " ADD %0,%0,#8\n" \ > " MOVT D0Ar2,#HI(1b)\n" \ > " JUMP D0Ar2,#LO(1b)\n" \ > " .previous\n" \ Ditto for all of those, actually. > +/* > + * Copy from user to kernel. The return-value is the number of bytes that were > + * inaccessible. > + */ > +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, > + unsigned long n) > { > register char *dst asm ("A0.2") = pdst; > register const char __user *src asm ("A1.2") = psrc; > @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, > __asm_copy_from_user_1(dst, src, retn); > n--; > if (retn) > - goto copy_exception_bytes; > + return retn + n; > } > } Umm... What happens if psrc points to the last byte of an unmapped page, with psrc + 1 pointing to valid data? AFAICS, you'll get GETB fail, have retn increased to 1, n decremented by 1, then, assuming you end up in that byte-copy loop, have GETB at src + 1 *succeed*, the value fetched fed to SETB at dst + 1, n decremented again, non-zero retn finally spotted and n + retn returned, reporting that you've copied a single byte. Which you certainly have, but it's pdst[1] = psrc[1], not pdst[0] = psrc[0]. IOW, you need to check retn after all __asm_copy_from_user_{1,2} in the beginning. As for topic branch for #1 and #2 - sure, perfectly fine by me. Just give me the branch name and I'll pull. But I think the scenario above needs to be dealt with... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user @ 2017-04-05 6:54 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-05 6:54 UTC (permalink / raw) To: Al Viro; +Cc: linux-metag, stable [-- Attachment #1: Type: text/plain, Size: 2261 bytes --] On Tue, Apr 04, 2017 at 05:31:57PM +0100, Al Viro wrote: > On Tue, Apr 04, 2017 at 03:46:51PM +0100, James Hogan wrote: > > @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user); > > " GETB D1Ar1,[%1++]\n" \ > > "2: SETB [%0++],D1Ar1\n", \ > > "3: ADD %2,%2,#1\n" \ > > - " SETB [%0++],D1Ar1\n", \ > > + " ADD %0,%0,#1\n", \ > > Why bother incrementing dst here? Mainly for consistency with the current expectations of these macros (otherwise we'd break the unaligned handling of valid copies), but I think I agree that if the issue mentioned below if fixed before this patch we could remove these adds, since it'll always result in an immediate return based on n and retn (and not dst). > > +/* > > + * Copy from user to kernel. The return-value is the number of bytes that were > > + * inaccessible. > > + */ > > +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, > > + unsigned long n) > > { > > register char *dst asm ("A0.2") = pdst; > > register const char __user *src asm ("A1.2") = psrc; > > @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, > > __asm_copy_from_user_1(dst, src, retn); > > n--; > > if (retn) > > - goto copy_exception_bytes; > > + return retn + n; > > } > > } > > Umm... What happens if psrc points to the last byte of an unmapped page, > with psrc + 1 pointing to valid data? AFAICS, you'll get GETB fail, have > retn increased to 1, n decremented by 1, then, assuming you end up in that > byte-copy loop, have GETB at src + 1 *succeed*, the value fetched fed to SETB > at dst + 1, n decremented again, non-zero retn finally spotted and n + retn > returned, reporting that you've copied a single byte. Which you certainly > have, but it's pdst[1] = psrc[1], not pdst[0] = psrc[0]. > > IOW, you need to check retn after all __asm_copy_from_user_{1,2} in the > beginning. Agreed. This is one of the other fixes I mentioned. I'll rearrange to put that fix first. > As for topic branch for #1 and #2 - sure, perfectly fine by me. Just give > me the branch name and I'll pull. But I think the scenario above needs to > be dealt with... Thanks for reviewing Cheers James [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user @ 2017-04-05 6:54 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-05 6:54 UTC (permalink / raw) To: Al Viro; +Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA, stable-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 2261 bytes --] On Tue, Apr 04, 2017 at 05:31:57PM +0100, Al Viro wrote: > On Tue, Apr 04, 2017 at 03:46:51PM +0100, James Hogan wrote: > > @@ -618,7 +617,7 @@ EXPORT_SYMBOL(__copy_user); > > " GETB D1Ar1,[%1++]\n" \ > > "2: SETB [%0++],D1Ar1\n", \ > > "3: ADD %2,%2,#1\n" \ > > - " SETB [%0++],D1Ar1\n", \ > > + " ADD %0,%0,#1\n", \ > > Why bother incrementing dst here? Mainly for consistency with the current expectations of these macros (otherwise we'd break the unaligned handling of valid copies), but I think I agree that if the issue mentioned below if fixed before this patch we could remove these adds, since it'll always result in an immediate return based on n and retn (and not dst). > > +/* > > + * Copy from user to kernel. The return-value is the number of bytes that were > > + * inaccessible. > > + */ > > +unsigned long raw_copy_from_user(void *pdst, const void __user *psrc, > > + unsigned long n) > > { > > register char *dst asm ("A0.2") = pdst; > > register const char __user *src asm ("A1.2") = psrc; > > @@ -724,7 +721,7 @@ unsigned long __copy_user_zeroing(void *pdst, const void __user *psrc, > > __asm_copy_from_user_1(dst, src, retn); > > n--; > > if (retn) > > - goto copy_exception_bytes; > > + return retn + n; > > } > > } > > Umm... What happens if psrc points to the last byte of an unmapped page, > with psrc + 1 pointing to valid data? AFAICS, you'll get GETB fail, have > retn increased to 1, n decremented by 1, then, assuming you end up in that > byte-copy loop, have GETB at src + 1 *succeed*, the value fetched fed to SETB > at dst + 1, n decremented again, non-zero retn finally spotted and n + retn > returned, reporting that you've copied a single byte. Which you certainly > have, but it's pdst[1] = psrc[1], not pdst[0] = psrc[0]. > > IOW, you need to check retn after all __asm_copy_from_user_{1,2} in the > beginning. Agreed. This is one of the other fixes I mentioned. I'll rearrange to put that fix first. > As for topic branch for #1 and #2 - sure, perfectly fine by me. Just give > me the branch name and I'll pull. But I think the scenario above needs to > be dealt with... Thanks for reviewing Cheers James [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 801 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <cover.67b8034e25703fa0c68295f1ac1d7abe4bd1fac9.1491316836.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>]
* [PATCH 3/3] metag/usercopy: Switch to RAW_COPY_USER [not found] ` <cover.67b8034e25703fa0c68295f1ac1d7abe4bd1fac9.1491316836.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> @ 2017-04-04 14:46 ` James Hogan 0 siblings, 0 replies; 11+ messages in thread From: James Hogan @ 2017-04-04 14:46 UTC (permalink / raw) To: linux-metag-u79uwXL29TY76Z2rM5mHXA, Al Viro; +Cc: James Hogan Switch to using raw user copy instead of providing metag specific [__]copy_{to,from}_user[_inatomic](). This simplifies the metag uaccess.h and allows us to take advantage of extra checking in the generic versions. Signed-off-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org> Cc: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> Cc: linux-metag-u79uwXL29TY76Z2rM5mHXA@public.gmane.org --- arch/metag/Kconfig | 1 + arch/metag/include/asm/uaccess.h | 31 ++----------------------------- arch/metag/lib/usercopy.c | 6 +++--- 3 files changed, 6 insertions(+), 32 deletions(-) diff --git a/arch/metag/Kconfig b/arch/metag/Kconfig index 5b7a45d99cfb..ecce0c5ec8e8 100644 --- a/arch/metag/Kconfig +++ b/arch/metag/Kconfig @@ -1,5 +1,6 @@ config METAG def_bool y + select ARCH_HAS_RAW_COPY_USER select EMBEDDED select GENERIC_ATOMIC64 select GENERIC_CLOCKEVENTS diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h index 61805d9f9375..5ebc2850690e 100644 --- a/arch/metag/include/asm/uaccess.h +++ b/arch/metag/include/asm/uaccess.h @@ -174,35 +174,8 @@ extern long __must_check strnlen_user(const char __user *src, long count); extern unsigned long raw_copy_from_user(void *to, const void __user *from, unsigned long n); - -static inline unsigned long -copy_from_user(void *to, const void __user *from, unsigned long n) -{ - unsigned long res = n; - if (likely(access_ok(VERIFY_READ, from, n))) - res = raw_copy_from_user(to, from, n); - if (unlikely(res)) - memset(to + (n - res), 0, res); - return res; -} - -#define __copy_from_user(to, from, n) raw_copy_from_user(to, from, n) -#define __copy_from_user_inatomic __copy_from_user - -extern unsigned long __must_check __copy_user(void __user *to, - const void *from, - unsigned long n); - -static inline unsigned long copy_to_user(void __user *to, const void *from, - unsigned long n) -{ - if (access_ok(VERIFY_WRITE, to, n)) - return __copy_user(to, from, n); - return n; -} - -#define __copy_to_user(to, from, n) __copy_user(to, from, n) -#define __copy_to_user_inatomic __copy_to_user +extern unsigned long raw_copy_to_user(void __user *to, const void *from, + unsigned long n); /* * Zero Userspace diff --git a/arch/metag/lib/usercopy.c b/arch/metag/lib/usercopy.c index bb1f7d60fcc0..9c9aa2442b70 100644 --- a/arch/metag/lib/usercopy.c +++ b/arch/metag/lib/usercopy.c @@ -524,8 +524,8 @@ "SUB %1, %1, D0Ar2\n" \ "SUB %3, %3, D1Ar1\n") -unsigned long __copy_user(void __user *pdst, const void *psrc, - unsigned long n) +unsigned long raw_copy_to_user(void __user *pdst, const void *psrc, + unsigned long n) { register char __user *dst asm ("A0.2") = pdst; register const char *src asm ("A1.2") = psrc; @@ -610,7 +610,7 @@ unsigned long __copy_user(void __user *pdst, const void *psrc, return retn; } -EXPORT_SYMBOL(__copy_user); +EXPORT_SYMBOL(raw_copy_to_user); #define __asm_copy_from_user_1(to, from, ret) \ __asm_copy_user_cont(to, from, ret, \ -- git-series 0.8.10 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-04-05 6:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 14:46 [PATCH 0/3] metag/usercopy: Support RAW_COPY_USER James Hogan
2017-04-04 14:46 ` James Hogan
2017-04-04 14:46 ` [PATCH 1/3] metag/usercopy: Drop unused macros James Hogan
2017-04-04 14:46 ` James Hogan
2017-04-04 14:46 ` [PATCH 2/3] metag/usercopy: Zero rest of buffer from copy_from_user James Hogan
2017-04-04 14:46 ` James Hogan
2017-04-04 16:31 ` Al Viro
2017-04-04 16:31 ` Al Viro
2017-04-05 6:54 ` James Hogan
2017-04-05 6:54 ` James Hogan
[not found] ` <cover.67b8034e25703fa0c68295f1ac1d7abe4bd1fac9.1491316836.git-series.james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
2017-04-04 14:46 ` [PATCH 3/3] metag/usercopy: Switch to RAW_COPY_USER James Hogan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.