[PATCH backport v3.15..v4.1 0/2] MIPS: uaccess: EVA fixes

[PATCH backport v3.15..v4.1 0/2] MIPS: uaccess: EVA fixes

[James Hogan]

These two backports fix bugs in the MIPS uaccess functions for MIPS
Enhanced Virtual Addressing (EVA), which were added in v3.15.

The upstream commits don't work directly when applied to kernels older
than v4.2 where the eva_kernel_access() function was introduced, so we
instead use segment_eq() directly, along with config_enabled(CONFIG_EVA)
where necessary to avoid module symbol errors.

James Hogan (2):
  MIPS: uaccess: Take EVA into account in __copy_from_user()
  MIPS: uaccess: Take EVA into account in [__]clear_user

 arch/mips/include/asm/uaccess.h | 44 +++++++++++++++++++++++++++++------------
 arch/mips/kernel/mips_ksyms.c   |  2 ++
 arch/mips/lib/memset.S          |  2 ++
 3 files changed, 35 insertions(+), 13 deletions(-)

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Markos Chandras <markos.chandras@imgtec.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Leonid Yegoshin <leonid.yegoshin@imgtec.com>
Cc: linux-mips@linux-mips.org
-- 
2.4.10

[PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user()

[James Hogan]

commit 6f06a2c45d8d714ea3b11a360b4a7191e52acaa4 upstream.

When EVA is in use, __copy_from_user() was unconditionally using the EVA
instructions to read the user address space, however this can also be
used for kernel access. If the address isn't a valid user address it
will cause an address error or TLB exception, and if it is then user
memory may be read instead of kernel memory.

For example in the following stack trace from Linux v3.10 (changes since
then will prevent this particular one still happening) kernel_sendmsg()
set the user address limit to KERNEL_DS, and tcp_sendmsg() goes on to
use __copy_from_user() with a kernel address in KSeg0.

[<8002d434>] __copy_fromuser_common+0x10c/0x254
[<805710e0>] tcp_sendmsg+0x5f4/0xf00
[<804e8e3c>] sock_sendmsg+0x78/0xa0
[<804e8f28>] kernel_sendmsg+0x24/0x38
[<804ee0f8>] sock_no_sendpage+0x70/0x7c
[<8017c820>] pipe_to_sendpage+0x80/0x98
[<8017c6b0>] splice_from_pipe_feed+0xa8/0x198
[<8017cc54>] __splice_from_pipe+0x4c/0x8c
[<8017e844>] splice_from_pipe+0x58/0x78
[<8017e884>] generic_splice_sendpage+0x20/0x2c
[<8017d690>] do_splice_from+0xb4/0x110
[<8017d710>] direct_splice_actor+0x24/0x30
[<8017d394>] splice_direct_to_actor+0xd8/0x208
[<8017d51c>] do_splice_direct+0x58/0x7c
[<8014eaf4>] do_sendfile+0x1dc/0x39c
[<8014f82c>] SyS_sendfile+0x90/0xf8

Add the eva_kernel_access() check in __copy_from_user() like the one in
copy_from_user().

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Markos Chandras <markos.chandras@imgtec.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Leonid Yegoshin <leonid.yegoshin@imgtec.com>
Cc: linux-mips@linux-mips.org
Patchwork: https://patchwork.linux-mips.org/patch/10843/
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
[james.hogan@imgtec.com: backport]
Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 arch/mips/include/asm/uaccess.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
index bf8b32450ef6..d8f7394275af 100644
--- a/arch/mips/include/asm/uaccess.h
+++ b/arch/mips/include/asm/uaccess.h
@@ -1095,9 +1095,15 @@ extern size_t __copy_in_user_eva(void *__to, const void *__from, size_t __n);
 	__cu_to = (to);							\
 	__cu_from = (from);						\
 	__cu_len = (n);							\
-	might_fault();							\
-	__cu_len = __invoke_copy_from_user(__cu_to, __cu_from,		\
-					   __cu_len);			\
+	if (segment_eq(get_fs(), get_ds())) {				\
+		__cu_len = __invoke_copy_from_kernel(__cu_to,		\
+						     __cu_from,		\
+						     __cu_len);		\
+	} else {							\
+		might_fault();						\
+		__cu_len = __invoke_copy_from_user(__cu_to, __cu_from,	\
+						   __cu_len);		\
+	}								\
 	__cu_len;							\
 })
 
-- 
2.4.10

Re: [PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user()

[Leonid Yegoshin]

On 01/04/2016 12:29 PM, James Hogan wrote:
> commit 6f06a2c45d8d714ea3b11a360b4a7191e52acaa4 upstream.
>
> When EVA is in use, __copy_from_user() was unconditionally using the EVA
> instructions to read the user address space, however this can also be
> used for kernel access. If the address isn't a valid user address it
> will cause an address error or TLB exception, and if it is then user
> memory may be read instead of kernel memory.
>
> For example in the following stack trace from Linux v3.10 (changes since
> then will prevent this particular one still happening) kernel_sendmsg()
> set the user address limit to KERNEL_DS, and tcp_sendmsg() goes on to
> use __copy_from_user() with a kernel address in KSeg0.
>
> [<8002d434>] __copy_fromuser_common+0x10c/0x254
> [<805710e0>] tcp_sendmsg+0x5f4/0xf00
> [<804e8e3c>] sock_sendmsg+0x78/0xa0
> [<804e8f28>] kernel_sendmsg+0x24/0x38
> [<804ee0f8>] sock_no_sendpage+0x70/0x7c
> [<8017c820>] pipe_to_sendpage+0x80/0x98
> [<8017c6b0>] splice_from_pipe_feed+0xa8/0x198
> [<8017cc54>] __splice_from_pipe+0x4c/0x8c
> [<8017e844>] splice_from_pipe+0x58/0x78
> [<8017e884>] generic_splice_sendpage+0x20/0x2c
> [<8017d690>] do_splice_from+0xb4/0x110
> [<8017d710>] direct_splice_actor+0x24/0x30
> [<8017d394>] splice_direct_to_actor+0xd8/0x208
> [<8017d51c>] do_splice_direct+0x58/0x7c
> [<8014eaf4>] do_sendfile+0x1dc/0x39c
> [<8014f82c>] SyS_sendfile+0x90/0xf8
>
> Add the eva_kernel_access() check in __copy_from_user() like the one in
> copy_from_user().
>

I think that the best way to fix this problem is - stop 
skb_do_copy_data_nocache() using __copy_from_user_nocache(). All TCP/IP 
stuff (beyond SCTP) doesn't use "accelerated" __copy*() functions. 
Adding a user space check in __copy_from_user() kills the original 
design. And splitting a user space processing in two places 
(skb_do_copy_data_nocache() calls access_ok(), BTW) - and it is also a 
bad thing in my mind.

- Leonid.

Re: [PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user()

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

Hi Leonid,

On Mon, Jan 04, 2016 at 01:33:51PM -0800, Leonid Yegoshin wrote:
> On 01/04/2016 12:29 PM, James Hogan wrote:
> > Add the eva_kernel_access() check in __copy_from_user() like the one in
> > copy_from_user().

...

> Adding a user space check in __copy_from_user() kills the original 
> design.

The original patch which did the same thing is already merged, so its a
bit late to be arguing with it now.

In any case, like other __ prefixed uaccess functions I believe the
semantics are such that __copy_from_user() can be used instead of
copy_from_user() to avoid multiple redundant access_ok() checks, since
the caller can do it once before calling __copy_from_user().

I have yet to see evidence or documentation suggesting that it was
intended never to be used for kernel addresses, which would be
inconsistent with copy_from_user and other __ uaccess functions which do
handle them. Given the awkwardness of auditing whether some of these
functions are ever called with kernel addresses, and the rate of code
change in Linux, taking shortcuts with the semantics, even if possible
to do at this moment, will only result in future code rot.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH backport v3.15..v4.1 1/2] MIPS: uaccess: Take EVA into account in __copy_from_user()

[Leonid Yegoshin]

On 01/04/2016 02:28 PM, James Hogan wrote:
> Hi Leonid,
>
> On Mon, Jan 04, 2016 at 01:33:51PM -0800, Leonid Yegoshin wrote:
>> On 01/04/2016 12:29 PM, James Hogan wrote:
>>> Add the eva_kernel_access() check in __copy_from_user() like the one in
>>> copy_from_user().
> ...
>
>> Adding a user space check in __copy_from_user() kills the original
>> design.
> The original patch which did the same thing is already merged, so its a
> bit late to be arguing with it now.
>
> In any case, like other __ prefixed uaccess functions I believe the
> semantics are such that __copy_from_user() can be used instead of
> copy_from_user() to avoid multiple redundant access_ok() checks, since
> the caller can do it once before calling __copy_from_user().

... and it seems ridiculous that all net code uses copy_from*() besides 
one place which uses __copy_from_user_nocache() right after access_ok(). 
Note - there is no any saving because of splitting address verification 
into access_ok() from copy*() in this specific case.


>
> I have yet to see evidence or documentation suggesting that it was
> intended never to be used for kernel addresses, which would be
> inconsistent with copy_from_user and other __ uaccess functions which do
> handle them. Given the awkwardness of auditing whether some of these
> functions are ever called with kernel addresses, and the rate of code
> change in Linux, taking shortcuts with the semantics, even if possible
> to do at this moment, will only result in future code rot.

Well, there are cases then you know inside caller that address is kernel 
address space and wants just skip ds set/restore and access_ok(). But it 
is not a case of skb_do_copy_data_nocache().

- Leonid.

>
> Cheers
> James

[PATCH backport v3.15..v4.1 2/2] MIPS: uaccess: Take EVA into account in [__]clear_user

[James Hogan]

commit d6a428fb583738ad685c91a684748cdee7b2a05f upstream.

__clear_user() (and clear_user() which uses it), always access the user
mode address space, which results in EVA store instructions when EVA is
enabled even if the current user address limit is KERNEL_DS.

Fix this by adding a new symbol __bzero_kernel for the normal kernel
address space bzero in EVA mode, and call that from __clear_user() if
eva_kernel_access().

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Markos Chandras <markos.chandras@imgtec.com>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Leonid Yegoshin <leonid.yegoshin@imgtec.com>
Cc: linux-mips@linux-mips.org
Patchwork: https://patchwork.linux-mips.org/patch/10844/
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
[james.hogan@imgtec.com: backport]
Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 arch/mips/include/asm/uaccess.h | 32 ++++++++++++++++++++++----------
 arch/mips/kernel/mips_ksyms.c   |  2 ++
 arch/mips/lib/memset.S          |  2 ++
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/arch/mips/include/asm/uaccess.h b/arch/mips/include/asm/uaccess.h
index d8f7394275af..2d4118499519 100644
--- a/arch/mips/include/asm/uaccess.h
+++ b/arch/mips/include/asm/uaccess.h
@@ -1207,16 +1207,28 @@ __clear_user(void __user *addr, __kernel_size_t size)
 {
 	__kernel_size_t res;
 
-	might_fault();
-	__asm__ __volatile__(
-		"move\t$4, %1\n\t"
-		"move\t$5, $0\n\t"
-		"move\t$6, %2\n\t"
-		__MODULE_JAL(__bzero)
-		"move\t%0, $6"
-		: "=r" (res)
-		: "r" (addr), "r" (size)
-		: "$4", "$5", "$6", __UA_t0, __UA_t1, "$31");
+	if (config_enabled(CONFIG_EVA) && segment_eq(get_fs(), get_ds())) {
+		__asm__ __volatile__(
+			"move\t$4, %1\n\t"
+			"move\t$5, $0\n\t"
+			"move\t$6, %2\n\t"
+			__MODULE_JAL(__bzero_kernel)
+			"move\t%0, $6"
+			: "=r" (res)
+			: "r" (addr), "r" (size)
+			: "$4", "$5", "$6", __UA_t0, __UA_t1, "$31");
+	} else {
+		might_fault();
+		__asm__ __volatile__(
+			"move\t$4, %1\n\t"
+			"move\t$5, $0\n\t"
+			"move\t$6, %2\n\t"
+			__MODULE_JAL(__bzero)
+			"move\t%0, $6"
+			: "=r" (res)
+			: "r" (addr), "r" (size)
+			: "$4", "$5", "$6", __UA_t0, __UA_t1, "$31");
+	}
 
 	return res;
 }
diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
index 291af0b5c482..e2b6ab74643d 100644
--- a/arch/mips/kernel/mips_ksyms.c
+++ b/arch/mips/kernel/mips_ksyms.c
@@ -17,6 +17,7 @@
 #include <asm/fpu.h>
 #include <asm/msa.h>
 
+extern void *__bzero_kernel(void *__s, size_t __count);
 extern void *__bzero(void *__s, size_t __count);
 extern long __strncpy_from_kernel_nocheck_asm(char *__to,
 					      const char *__from, long __len);
@@ -64,6 +65,7 @@ EXPORT_SYMBOL(__copy_from_user_eva);
 EXPORT_SYMBOL(__copy_in_user_eva);
 EXPORT_SYMBOL(__copy_to_user_eva);
 EXPORT_SYMBOL(__copy_user_inatomic_eva);
+EXPORT_SYMBOL(__bzero_kernel);
 #endif
 EXPORT_SYMBOL(__bzero);
 EXPORT_SYMBOL(__strncpy_from_kernel_nocheck_asm);
diff --git a/arch/mips/lib/memset.S b/arch/mips/lib/memset.S
index b8e63fd00375..8f0019a2e5c8 100644
--- a/arch/mips/lib/memset.S
+++ b/arch/mips/lib/memset.S
@@ -283,6 +283,8 @@ LEAF(memset)
 1:
 #ifndef CONFIG_EVA
 FEXPORT(__bzero)
+#else
+FEXPORT(__bzero_kernel)
 #endif
 	__BUILD_BZERO LEGACY_MODE
 
-- 
2.4.10

Re: [PATCH backport v3.15..v4.1 0/2] MIPS: uaccess: EVA fixes

[Luis Henriques]

On Mon, Jan 04, 2016 at 08:29:02PM +0000, James Hogan wrote:
> These two backports fix bugs in the MIPS uaccess functions for MIPS
> Enhanced Virtual Addressing (EVA), which were added in v3.15.
> 
> The upstream commits don't work directly when applied to kernels older
> than v4.2 where the eva_kernel_access() function was introduced, so we
> instead use segment_eq() directly, along with config_enabled(CONFIG_EVA)
> where necessary to avoid module symbol errors.
>

Thanks James, I'll queue both for the next 3.16 kernel.

Cheers,
--
Lu�s


> James Hogan (2):
>   MIPS: uaccess: Take EVA into account in __copy_from_user()
>   MIPS: uaccess: Take EVA into account in [__]clear_user
> 
>  arch/mips/include/asm/uaccess.h | 44 +++++++++++++++++++++++++++++------------
>  arch/mips/kernel/mips_ksyms.c   |  2 ++
>  arch/mips/lib/memset.S          |  2 ++
>  3 files changed, 35 insertions(+), 13 deletions(-)
> 
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Markos Chandras <markos.chandras@imgtec.com>
> Cc: Paul Burton <paul.burton@imgtec.com>
> Cc: Leonid Yegoshin <leonid.yegoshin@imgtec.com>
> Cc: linux-mips@linux-mips.org
> -- 
> 2.4.10
> 
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html