* [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-01-09 16:52 ` James Cowgill
0 siblings, 0 replies; 8+ messages in thread
From: James Cowgill @ 2017-01-09 16:52 UTC (permalink / raw)
To: linux-mips; +Cc: Ralf Baechle
If copy_from_user is called with a large buffer (>= 128 bytes) and the
userspace buffer refers partially to unreadable memory, then it is
possible for Octeon's copy_from_user to report the wrong number of bytes
have been copied. In the case where the buffer size is an exact multiple
of 128 and the fault occurs in the last 64 bytes, copy_from_user will
report that all the bytes were copied successfully but leave some
garbage in the destination buffer.
The bug is in the main __copy_user_common loop in octeon-memcpy.S where
in the middle of the loop, src and dst are incremented by 128 bytes. The
l_exc_copy fault handler is used after this but that assumes that
"src < THREAD_BUADDR($28)". This is not the case if src has already been
incremented.
Fix by adding an extra fault handler which rewinds the src and dst
pointers 128 bytes before falling though to l_exc_copy.
Thanks to the pwritev test from the strace test suite for originally
highlighting this bug!
Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
Cc: stable@vger.kernel.org
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
---
arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
index 64e08df51d65..4668537b09c2 100644
--- a/arch/mips/cavium-octeon/octeon-memcpy.S
+++ b/arch/mips/cavium-octeon/octeon-memcpy.S
@@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
ADD src, src, 16*NBYTES
EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
ADD dst, dst, 16*NBYTES
-EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
-EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
-EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
-EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
+EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
+EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
+EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
+EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
-EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
-EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
-EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
-EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
+EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
+EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
+EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
+EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
@@ -383,6 +383,10 @@ done:
nop
END(memcpy)
+l_exc_copy_rewind16:
+ /* Rewind src and dst by 16*NBYTES for l_exc_copy */
+ SUB src, src, 16*NBYTES
+ SUB dst, dst, 16*NBYTES
l_exc_copy:
/*
* Copy bytes from src until faulting load address (or until a
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-01-09 16:52 ` James Cowgill
0 siblings, 0 replies; 8+ messages in thread
From: James Cowgill @ 2017-01-09 16:52 UTC (permalink / raw)
To: linux-mips; +Cc: Ralf Baechle
If copy_from_user is called with a large buffer (>= 128 bytes) and the
userspace buffer refers partially to unreadable memory, then it is
possible for Octeon's copy_from_user to report the wrong number of bytes
have been copied. In the case where the buffer size is an exact multiple
of 128 and the fault occurs in the last 64 bytes, copy_from_user will
report that all the bytes were copied successfully but leave some
garbage in the destination buffer.
The bug is in the main __copy_user_common loop in octeon-memcpy.S where
in the middle of the loop, src and dst are incremented by 128 bytes. The
l_exc_copy fault handler is used after this but that assumes that
"src < THREAD_BUADDR($28)". This is not the case if src has already been
incremented.
Fix by adding an extra fault handler which rewinds the src and dst
pointers 128 bytes before falling though to l_exc_copy.
Thanks to the pwritev test from the strace test suite for originally
highlighting this bug!
Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
Cc: stable@vger.kernel.org
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-mips@linux-mips.org
---
arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
index 64e08df51d65..4668537b09c2 100644
--- a/arch/mips/cavium-octeon/octeon-memcpy.S
+++ b/arch/mips/cavium-octeon/octeon-memcpy.S
@@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
ADD src, src, 16*NBYTES
EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
ADD dst, dst, 16*NBYTES
-EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
-EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
-EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
-EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
+EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
+EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
+EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
+EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
-EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
-EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
-EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
-EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
+EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
+EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
+EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
+EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
@@ -383,6 +383,10 @@ done:
nop
END(memcpy)
+l_exc_copy_rewind16:
+ /* Rewind src and dst by 16*NBYTES for l_exc_copy */
+ SUB src, src, 16*NBYTES
+ SUB dst, dst, 16*NBYTES
l_exc_copy:
/*
* Copy bytes from src until faulting load address (or until a
--
2.11.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-01-09 18:53 ` David Daney
0 siblings, 0 replies; 8+ messages in thread
From: David Daney @ 2017-01-09 18:53 UTC (permalink / raw)
To: James Cowgill, linux-mips, Ralf Baechle
On 01/09/2017 08:52 AM, James Cowgill wrote:
> If copy_from_user is called with a large buffer (>= 128 bytes) and the
> userspace buffer refers partially to unreadable memory, then it is
> possible for Octeon's copy_from_user to report the wrong number of bytes
> have been copied. In the case where the buffer size is an exact multiple
> of 128 and the fault occurs in the last 64 bytes, copy_from_user will
> report that all the bytes were copied successfully but leave some
> garbage in the destination buffer.
Yikes!
>
> The bug is in the main __copy_user_common loop in octeon-memcpy.S where
> in the middle of the loop, src and dst are incremented by 128 bytes. The
> l_exc_copy fault handler is used after this but that assumes that
> "src < THREAD_BUADDR($28)". This is not the case if src has already been
> incremented.
>
> Fix by adding an extra fault handler which rewinds the src and dst
> pointers 128 bytes before falling though to l_exc_copy.
>
> Thanks to the pwritev test from the strace test suite for originally
> highlighting this bug!
>
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
Good catch,
Acked-by: David Daney <david.daney@cavium.com>
> Cc: stable@vger.kernel.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
> ---
> arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
> index 64e08df51d65..4668537b09c2 100644
> --- a/arch/mips/cavium-octeon/octeon-memcpy.S
> +++ b/arch/mips/cavium-octeon/octeon-memcpy.S
> @@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
> ADD src, src, 16*NBYTES
> EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
> ADD dst, dst, 16*NBYTES
> -EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
> EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
> EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
> EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
> -EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
> EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
> EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
> @@ -383,6 +383,10 @@ done:
> nop
> END(memcpy)
>
> +l_exc_copy_rewind16:
> + /* Rewind src and dst by 16*NBYTES for l_exc_copy */
> + SUB src, src, 16*NBYTES
> + SUB dst, dst, 16*NBYTES
> l_exc_copy:
> /*
> * Copy bytes from src until faulting load address (or until a
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-01-09 18:53 ` David Daney
0 siblings, 0 replies; 8+ messages in thread
From: David Daney @ 2017-01-09 18:53 UTC (permalink / raw)
To: James Cowgill, linux-mips, Ralf Baechle
On 01/09/2017 08:52 AM, James Cowgill wrote:
> If copy_from_user is called with a large buffer (>= 128 bytes) and the
> userspace buffer refers partially to unreadable memory, then it is
> possible for Octeon's copy_from_user to report the wrong number of bytes
> have been copied. In the case where the buffer size is an exact multiple
> of 128 and the fault occurs in the last 64 bytes, copy_from_user will
> report that all the bytes were copied successfully but leave some
> garbage in the destination buffer.
Yikes!
>
> The bug is in the main __copy_user_common loop in octeon-memcpy.S where
> in the middle of the loop, src and dst are incremented by 128 bytes. The
> l_exc_copy fault handler is used after this but that assumes that
> "src < THREAD_BUADDR($28)". This is not the case if src has already been
> incremented.
>
> Fix by adding an extra fault handler which rewinds the src and dst
> pointers 128 bytes before falling though to l_exc_copy.
>
> Thanks to the pwritev test from the strace test suite for originally
> highlighting this bug!
>
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
Good catch,
Acked-by: David Daney <david.daney@cavium.com>
> Cc: stable@vger.kernel.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
> ---
> arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
> index 64e08df51d65..4668537b09c2 100644
> --- a/arch/mips/cavium-octeon/octeon-memcpy.S
> +++ b/arch/mips/cavium-octeon/octeon-memcpy.S
> @@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
> ADD src, src, 16*NBYTES
> EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
> ADD dst, dst, 16*NBYTES
> -EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
> EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
> EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
> EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
> -EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
> EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
> EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
> @@ -383,6 +383,10 @@ done:
> nop
> END(memcpy)
>
> +l_exc_copy_rewind16:
> + /* Rewind src and dst by 16*NBYTES for l_exc_copy */
> + SUB src, src, 16*NBYTES
> + SUB dst, dst, 16*NBYTES
> l_exc_copy:
> /*
> * Copy bytes from src until faulting load address (or until a
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-01-09 20:41 ` James Hogan
0 siblings, 0 replies; 8+ messages in thread
From: James Hogan @ 2017-01-09 20:41 UTC (permalink / raw)
To: James Cowgill; +Cc: linux-mips, Ralf Baechle
[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]
On Mon, Jan 09, 2017 at 04:52:28PM +0000, James Cowgill wrote:
> If copy_from_user is called with a large buffer (>= 128 bytes) and the
> userspace buffer refers partially to unreadable memory, then it is
> possible for Octeon's copy_from_user to report the wrong number of bytes
> have been copied. In the case where the buffer size is an exact multiple
> of 128 and the fault occurs in the last 64 bytes, copy_from_user will
> report that all the bytes were copied successfully but leave some
> garbage in the destination buffer.
>
> The bug is in the main __copy_user_common loop in octeon-memcpy.S where
> in the middle of the loop, src and dst are incremented by 128 bytes. The
> l_exc_copy fault handler is used after this but that assumes that
> "src < THREAD_BUADDR($28)". This is not the case if src has already been
> incremented.
>
> Fix by adding an extra fault handler which rewinds the src and dst
> pointers 128 bytes before falling though to l_exc_copy.
>
> Thanks to the pwritev test from the strace test suite for originally
> highlighting this bug!
>
May I suggest adding:
Fixes: 5b3b16880f40 ("MIPS: Add Cavium OCTEON processor support ...")
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
> Cc: stable@vger.kernel.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
Reviewed-by: James Hogan <james.hogan@imgtec.com>
Cheers
James
> ---
> arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
> index 64e08df51d65..4668537b09c2 100644
> --- a/arch/mips/cavium-octeon/octeon-memcpy.S
> +++ b/arch/mips/cavium-octeon/octeon-memcpy.S
> @@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
> ADD src, src, 16*NBYTES
> EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
> ADD dst, dst, 16*NBYTES
> -EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
> EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
> EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
> EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
> -EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
> EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
> EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
> @@ -383,6 +383,10 @@ done:
> nop
> END(memcpy)
>
> +l_exc_copy_rewind16:
> + /* Rewind src and dst by 16*NBYTES for l_exc_copy */
> + SUB src, src, 16*NBYTES
> + SUB dst, dst, 16*NBYTES
> l_exc_copy:
> /*
> * Copy bytes from src until faulting load address (or until a
> --
> 2.11.0
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-01-09 20:41 ` James Hogan
0 siblings, 0 replies; 8+ messages in thread
From: James Hogan @ 2017-01-09 20:41 UTC (permalink / raw)
To: James Cowgill; +Cc: linux-mips, Ralf Baechle
[-- Attachment #1: Type: text/plain, Size: 3397 bytes --]
On Mon, Jan 09, 2017 at 04:52:28PM +0000, James Cowgill wrote:
> If copy_from_user is called with a large buffer (>= 128 bytes) and the
> userspace buffer refers partially to unreadable memory, then it is
> possible for Octeon's copy_from_user to report the wrong number of bytes
> have been copied. In the case where the buffer size is an exact multiple
> of 128 and the fault occurs in the last 64 bytes, copy_from_user will
> report that all the bytes were copied successfully but leave some
> garbage in the destination buffer.
>
> The bug is in the main __copy_user_common loop in octeon-memcpy.S where
> in the middle of the loop, src and dst are incremented by 128 bytes. The
> l_exc_copy fault handler is used after this but that assumes that
> "src < THREAD_BUADDR($28)". This is not the case if src has already been
> incremented.
>
> Fix by adding an extra fault handler which rewinds the src and dst
> pointers 128 bytes before falling though to l_exc_copy.
>
> Thanks to the pwritev test from the strace test suite for originally
> highlighting this bug!
>
May I suggest adding:
Fixes: 5b3b16880f40 ("MIPS: Add Cavium OCTEON processor support ...")
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
> Cc: stable@vger.kernel.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
Reviewed-by: James Hogan <james.hogan@imgtec.com>
Cheers
James
> ---
> arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
> index 64e08df51d65..4668537b09c2 100644
> --- a/arch/mips/cavium-octeon/octeon-memcpy.S
> +++ b/arch/mips/cavium-octeon/octeon-memcpy.S
> @@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
> ADD src, src, 16*NBYTES
> EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
> ADD dst, dst, 16*NBYTES
> -EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
> EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
> EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
> EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
> -EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
> EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
> EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
> @@ -383,6 +383,10 @@ done:
> nop
> END(memcpy)
>
> +l_exc_copy_rewind16:
> + /* Rewind src and dst by 16*NBYTES for l_exc_copy */
> + SUB src, src, 16*NBYTES
> + SUB dst, dst, 16*NBYTES
> l_exc_copy:
> /*
> * Copy bytes from src until faulting load address (or until a
> --
> 2.11.0
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-02-11 21:30 ` James Hogan
0 siblings, 0 replies; 8+ messages in thread
From: James Hogan @ 2017-02-11 21:30 UTC (permalink / raw)
To: James Cowgill; +Cc: linux-mips, Ralf Baechle
[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]
On Mon, Jan 09, 2017 at 04:52:28PM +0000, James Cowgill wrote:
> If copy_from_user is called with a large buffer (>= 128 bytes) and the
> userspace buffer refers partially to unreadable memory, then it is
> possible for Octeon's copy_from_user to report the wrong number of bytes
> have been copied. In the case where the buffer size is an exact multiple
> of 128 and the fault occurs in the last 64 bytes, copy_from_user will
> report that all the bytes were copied successfully but leave some
> garbage in the destination buffer.
>
> The bug is in the main __copy_user_common loop in octeon-memcpy.S where
> in the middle of the loop, src and dst are incremented by 128 bytes. The
> l_exc_copy fault handler is used after this but that assumes that
> "src < THREAD_BUADDR($28)". This is not the case if src has already been
> incremented.
>
> Fix by adding an extra fault handler which rewinds the src and dst
> pointers 128 bytes before falling though to l_exc_copy.
>
> Thanks to the pwritev test from the strace test suite for originally
> highlighting this bug!
>
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
> Cc: stable@vger.kernel.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
Applied
Thanks
James
> ---
> arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
> index 64e08df51d65..4668537b09c2 100644
> --- a/arch/mips/cavium-octeon/octeon-memcpy.S
> +++ b/arch/mips/cavium-octeon/octeon-memcpy.S
> @@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
> ADD src, src, 16*NBYTES
> EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
> ADD dst, dst, 16*NBYTES
> -EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
> EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
> EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
> EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
> -EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
> EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
> EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
> @@ -383,6 +383,10 @@ done:
> nop
> END(memcpy)
>
> +l_exc_copy_rewind16:
> + /* Rewind src and dst by 16*NBYTES for l_exc_copy */
> + SUB src, src, 16*NBYTES
> + SUB dst, dst, 16*NBYTES
> l_exc_copy:
> /*
> * Copy bytes from src until faulting load address (or until a
> --
> 2.11.0
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers
@ 2017-02-11 21:30 ` James Hogan
0 siblings, 0 replies; 8+ messages in thread
From: James Hogan @ 2017-02-11 21:30 UTC (permalink / raw)
To: James Cowgill; +Cc: linux-mips, Ralf Baechle
[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]
On Mon, Jan 09, 2017 at 04:52:28PM +0000, James Cowgill wrote:
> If copy_from_user is called with a large buffer (>= 128 bytes) and the
> userspace buffer refers partially to unreadable memory, then it is
> possible for Octeon's copy_from_user to report the wrong number of bytes
> have been copied. In the case where the buffer size is an exact multiple
> of 128 and the fault occurs in the last 64 bytes, copy_from_user will
> report that all the bytes were copied successfully but leave some
> garbage in the destination buffer.
>
> The bug is in the main __copy_user_common loop in octeon-memcpy.S where
> in the middle of the loop, src and dst are incremented by 128 bytes. The
> l_exc_copy fault handler is used after this but that assumes that
> "src < THREAD_BUADDR($28)". This is not the case if src has already been
> incremented.
>
> Fix by adding an extra fault handler which rewinds the src and dst
> pointers 128 bytes before falling though to l_exc_copy.
>
> Thanks to the pwritev test from the strace test suite for originally
> highlighting this bug!
>
> Signed-off-by: James Cowgill <James.Cowgill@imgtec.com>
> Cc: stable@vger.kernel.org
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-mips@linux-mips.org
Applied
Thanks
James
> ---
> arch/mips/cavium-octeon/octeon-memcpy.S | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/arch/mips/cavium-octeon/octeon-memcpy.S b/arch/mips/cavium-octeon/octeon-memcpy.S
> index 64e08df51d65..4668537b09c2 100644
> --- a/arch/mips/cavium-octeon/octeon-memcpy.S
> +++ b/arch/mips/cavium-octeon/octeon-memcpy.S
> @@ -208,18 +208,18 @@ EXC( STORE t2, UNIT(6)(dst), s_exc_p10u)
> ADD src, src, 16*NBYTES
> EXC( STORE t3, UNIT(7)(dst), s_exc_p9u)
> ADD dst, dst, 16*NBYTES
> -EXC( LOAD t0, UNIT(-8)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-7)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-6)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-5)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-8)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-7)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-6)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-5)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-8)(dst), s_exc_p8u)
> EXC( STORE t1, UNIT(-7)(dst), s_exc_p7u)
> EXC( STORE t2, UNIT(-6)(dst), s_exc_p6u)
> EXC( STORE t3, UNIT(-5)(dst), s_exc_p5u)
> -EXC( LOAD t0, UNIT(-4)(src), l_exc_copy)
> -EXC( LOAD t1, UNIT(-3)(src), l_exc_copy)
> -EXC( LOAD t2, UNIT(-2)(src), l_exc_copy)
> -EXC( LOAD t3, UNIT(-1)(src), l_exc_copy)
> +EXC( LOAD t0, UNIT(-4)(src), l_exc_copy_rewind16)
> +EXC( LOAD t1, UNIT(-3)(src), l_exc_copy_rewind16)
> +EXC( LOAD t2, UNIT(-2)(src), l_exc_copy_rewind16)
> +EXC( LOAD t3, UNIT(-1)(src), l_exc_copy_rewind16)
> EXC( STORE t0, UNIT(-4)(dst), s_exc_p4u)
> EXC( STORE t1, UNIT(-3)(dst), s_exc_p3u)
> EXC( STORE t2, UNIT(-2)(dst), s_exc_p2u)
> @@ -383,6 +383,10 @@ done:
> nop
> END(memcpy)
>
> +l_exc_copy_rewind16:
> + /* Rewind src and dst by 16*NBYTES for l_exc_copy */
> + SUB src, src, 16*NBYTES
> + SUB dst, dst, 16*NBYTES
> l_exc_copy:
> /*
> * Copy bytes from src until faulting load address (or until a
> --
> 2.11.0
>
>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-11 21:31 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-09 16:52 [PATCH] MIPS: OCTEON: Fix copy_from_user fault handling for large buffers James Cowgill
2017-01-09 16:52 ` James Cowgill
2017-01-09 18:53 ` David Daney
2017-01-09 18:53 ` David Daney
2017-01-09 20:41 ` James Hogan
2017-01-09 20:41 ` James Hogan
2017-02-11 21:30 ` James Hogan
2017-02-11 21:30 ` 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.