* [PATCH] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
@ 2016-04-21 15:39 ` Paul Burton
0 siblings, 0 replies; 7+ messages in thread
From: Paul Burton @ 2016-04-21 15:39 UTC (permalink / raw)
To: linux-mips, Ralf Baechle
Cc: Michal Toman, Paul Burton, James Hogan, stable # v4 . 3+
If a kernel doesn't support MSA context (ie. CONFIG_CPU_HAS_MSA=n) then
it will only keep 64 bits per FP register in thread context, and the
calls to set_fpr64 in restore_msa_extcontext will overrun the end of the
FP register context into the FCSR & MSACSR values. GCC 6.x has become
smart enough to detect this & complain like so:
arch/mips/kernel/signal.c: In function 'protected_restore_fp_context':
./arch/mips/include/asm/processor.h:114:17: error: array subscript is above array bounds [-Werror=array-bounds]
fpr->val##width[FPR_IDX(width, idx)] = val; \
~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
./arch/mips/include/asm/processor.h:118:1: note: in expansion of macro 'BUILD_FPR_ACCESS'
BUILD_FPR_ACCESS(64)
The only way to trigger this code to run would be for a program to set
up an artificial extended MSA context structure following a sigframe &
execute sigreturn. Whilst this doesn't allow a program to write to any
state that it couldn't already, it makes little sense to allow this
"restoration" of MSA context in a system that doesn't support MSA.
Fix this by killing a program with SIGSYS if it tries something as crazy
as "restoring" fake MSA context in this way, also fixing the build error
& allowing for most of restore_msa_extcontext to be optimised out of
kernels without support for MSA.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Reported-by: Michal Toman <michal.toman@imgtec.com>
Fixes: bf82cb30c7e5 ("MIPS: Save MSA extended context around signals")
Cc: James Hogan <james.hogan@imgtec.com>
Cc: stable <stable@vger.kernel.org> # v4.3+
---
arch/mips/kernel/signal.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index bf792e2..a304b70 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
unsigned int csr;
int i, err;
+ if (!config_enabled(CONFIG_CPU_HAS_MSA))
+ return SIGSYS;
+
if (size != sizeof(*msa))
return -EINVAL;
--
2.8.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
@ 2016-04-21 15:39 ` Paul Burton
0 siblings, 0 replies; 7+ messages in thread
From: Paul Burton @ 2016-04-21 15:39 UTC (permalink / raw)
To: linux-mips, Ralf Baechle
Cc: Michal Toman, Paul Burton, James Hogan, stable # v4 . 3+
If a kernel doesn't support MSA context (ie. CONFIG_CPU_HAS_MSA=n) then
it will only keep 64 bits per FP register in thread context, and the
calls to set_fpr64 in restore_msa_extcontext will overrun the end of the
FP register context into the FCSR & MSACSR values. GCC 6.x has become
smart enough to detect this & complain like so:
arch/mips/kernel/signal.c: In function 'protected_restore_fp_context':
./arch/mips/include/asm/processor.h:114:17: error: array subscript is above array bounds [-Werror=array-bounds]
fpr->val##width[FPR_IDX(width, idx)] = val; \
~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
./arch/mips/include/asm/processor.h:118:1: note: in expansion of macro 'BUILD_FPR_ACCESS'
BUILD_FPR_ACCESS(64)
The only way to trigger this code to run would be for a program to set
up an artificial extended MSA context structure following a sigframe &
execute sigreturn. Whilst this doesn't allow a program to write to any
state that it couldn't already, it makes little sense to allow this
"restoration" of MSA context in a system that doesn't support MSA.
Fix this by killing a program with SIGSYS if it tries something as crazy
as "restoring" fake MSA context in this way, also fixing the build error
& allowing for most of restore_msa_extcontext to be optimised out of
kernels without support for MSA.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Reported-by: Michal Toman <michal.toman@imgtec.com>
Fixes: bf82cb30c7e5 ("MIPS: Save MSA extended context around signals")
Cc: James Hogan <james.hogan@imgtec.com>
Cc: stable <stable@vger.kernel.org> # v4.3+
---
arch/mips/kernel/signal.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index bf792e2..a304b70 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
unsigned int csr;
int i, err;
+ if (!config_enabled(CONFIG_CPU_HAS_MSA))
+ return SIGSYS;
+
if (size != sizeof(*msa))
return -EINVAL;
--
2.8.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
@ 2016-04-21 16:41 ` Maciej W. Rozycki
0 siblings, 0 replies; 7+ messages in thread
From: Maciej W. Rozycki @ 2016-04-21 16:41 UTC (permalink / raw)
To: Paul Burton
Cc: linux-mips, Ralf Baechle, Michal Toman, James Hogan, stable # v4 . 3+
On Thu, 21 Apr 2016, Paul Burton wrote:
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index bf792e2..a304b70 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
> unsigned int csr;
> int i, err;
>
> + if (!config_enabled(CONFIG_CPU_HAS_MSA))
> + return SIGSYS;
> +
> if (size != sizeof(*msa))
> return -EINVAL;
The priciple of your change looks reasonable itself to me, however its
call site is ill-formed making it possible to return a nonsensical error
code:
if (used & USED_EXTCONTEXT)
err |= restore_extcontext(sc_to_extcontext(sc));
return err ?: sig;
}
(`restore_extcontext' takes the result from `restore_msa_extcontext' and
passes it on) so if an earlier call has set `err' to -EINVAL (which I take
it is the only value expected here or we'd have a preexisting problem),
then `protected_restore_fp_context' (which is where this code comes from)
will return (-EINVAL | SIGSYS).
So you need to redesign this code somehow I'm afraid, maybe just changing
the condition to:
if (!err && (used & USED_EXTCONTEXT))
will do (although while at it I'd double-check that `err' can really only
be -EINVAL here).
Maciej
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
@ 2016-04-21 16:41 ` Maciej W. Rozycki
0 siblings, 0 replies; 7+ messages in thread
From: Maciej W. Rozycki @ 2016-04-21 16:41 UTC (permalink / raw)
To: Paul Burton
Cc: linux-mips, Ralf Baechle, Michal Toman, James Hogan, stable # v4 . 3+
On Thu, 21 Apr 2016, Paul Burton wrote:
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index bf792e2..a304b70 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
> unsigned int csr;
> int i, err;
>
> + if (!config_enabled(CONFIG_CPU_HAS_MSA))
> + return SIGSYS;
> +
> if (size != sizeof(*msa))
> return -EINVAL;
The priciple of your change looks reasonable itself to me, however its
call site is ill-formed making it possible to return a nonsensical error
code:
if (used & USED_EXTCONTEXT)
err |= restore_extcontext(sc_to_extcontext(sc));
return err ?: sig;
}
(`restore_extcontext' takes the result from `restore_msa_extcontext' and
passes it on) so if an earlier call has set `err' to -EINVAL (which I take
it is the only value expected here or we'd have a preexisting problem),
then `protected_restore_fp_context' (which is where this code comes from)
will return (-EINVAL | SIGSYS).
So you need to redesign this code somehow I'm afraid, maybe just changing
the condition to:
if (!err && (used & USED_EXTCONTEXT))
will do (although while at it I'd double-check that `err' can really only
be -EINVAL here).
Maciej
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
@ 2016-04-21 17:04 ` Paul Burton
0 siblings, 0 replies; 7+ messages in thread
From: Paul Burton @ 2016-04-21 17:04 UTC (permalink / raw)
To: linux-mips, Ralf Baechle
Cc: Michal Toman, Paul Burton, James Hogan, stable # v4 . 3+
If a kernel doesn't support MSA context (ie. CONFIG_CPU_HAS_MSA=n) then
it will only keep 64 bits per FP register in thread context, and the
calls to set_fpr64 in restore_msa_extcontext will overrun the end of the
FP register context into the FCSR & MSACSR values. GCC 6.x has become
smart enough to detect this & complain like so:
arch/mips/kernel/signal.c: In function 'protected_restore_fp_context':
./arch/mips/include/asm/processor.h:114:17: error: array subscript is above array bounds [-Werror=array-bounds]
fpr->val##width[FPR_IDX(width, idx)] = val; \
~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
./arch/mips/include/asm/processor.h:118:1: note: in expansion of macro 'BUILD_FPR_ACCESS'
BUILD_FPR_ACCESS(64)
The only way to trigger this code to run would be for a program to set
up an artificial extended MSA context structure following a sigframe &
execute sigreturn. Whilst this doesn't allow a program to write to any
state that it couldn't already, it makes little sense to allow this
"restoration" of MSA context in a system that doesn't support MSA.
Fix this by killing a program with SIGSYS if it tries something as crazy
as "restoring" fake MSA context in this way, also fixing the build error
& allowing for most of restore_msa_extcontext to be optimised out of
kernels without support for MSA.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Reported-by: Michal Toman <michal.toman@imgtec.com>
Fixes: bf82cb30c7e5 ("MIPS: Save MSA extended context around signals")
Cc: James Hogan <james.hogan@imgtec.com>
Cc: stable <stable@vger.kernel.org> # v4.3+
---
Changes in v2:
- Prevent potential for malformed errno/signal from protected_restore_fp_context.
arch/mips/kernel/signal.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index bf792e2..fc7c1f0 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
unsigned int csr;
int i, err;
+ if (!config_enabled(CONFIG_CPU_HAS_MSA))
+ return SIGSYS;
+
if (size != sizeof(*msa))
return -EINVAL;
@@ -398,8 +401,8 @@ int protected_restore_fp_context(void __user *sc)
}
fp_done:
- if (used & USED_EXTCONTEXT)
- err |= restore_extcontext(sc_to_extcontext(sc));
+ if (!err && (used & USED_EXTCONTEXT))
+ err = restore_extcontext(sc_to_extcontext(sc));
return err ?: sig;
}
--
2.8.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
@ 2016-04-21 17:04 ` Paul Burton
0 siblings, 0 replies; 7+ messages in thread
From: Paul Burton @ 2016-04-21 17:04 UTC (permalink / raw)
To: linux-mips, Ralf Baechle
Cc: Michal Toman, Paul Burton, James Hogan, stable # v4 . 3+
If a kernel doesn't support MSA context (ie. CONFIG_CPU_HAS_MSA=n) then
it will only keep 64 bits per FP register in thread context, and the
calls to set_fpr64 in restore_msa_extcontext will overrun the end of the
FP register context into the FCSR & MSACSR values. GCC 6.x has become
smart enough to detect this & complain like so:
arch/mips/kernel/signal.c: In function 'protected_restore_fp_context':
./arch/mips/include/asm/processor.h:114:17: error: array subscript is above array bounds [-Werror=array-bounds]
fpr->val##width[FPR_IDX(width, idx)] = val; \
~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
./arch/mips/include/asm/processor.h:118:1: note: in expansion of macro 'BUILD_FPR_ACCESS'
BUILD_FPR_ACCESS(64)
The only way to trigger this code to run would be for a program to set
up an artificial extended MSA context structure following a sigframe &
execute sigreturn. Whilst this doesn't allow a program to write to any
state that it couldn't already, it makes little sense to allow this
"restoration" of MSA context in a system that doesn't support MSA.
Fix this by killing a program with SIGSYS if it tries something as crazy
as "restoring" fake MSA context in this way, also fixing the build error
& allowing for most of restore_msa_extcontext to be optimised out of
kernels without support for MSA.
Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Reported-by: Michal Toman <michal.toman@imgtec.com>
Fixes: bf82cb30c7e5 ("MIPS: Save MSA extended context around signals")
Cc: James Hogan <james.hogan@imgtec.com>
Cc: stable <stable@vger.kernel.org> # v4.3+
---
Changes in v2:
- Prevent potential for malformed errno/signal from protected_restore_fp_context.
arch/mips/kernel/signal.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index bf792e2..fc7c1f0 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
unsigned int csr;
int i, err;
+ if (!config_enabled(CONFIG_CPU_HAS_MSA))
+ return SIGSYS;
+
if (size != sizeof(*msa))
return -EINVAL;
@@ -398,8 +401,8 @@ int protected_restore_fp_context(void __user *sc)
}
fp_done:
- if (used & USED_EXTCONTEXT)
- err |= restore_extcontext(sc_to_extcontext(sc));
+ if (!err && (used & USED_EXTCONTEXT))
+ err = restore_extcontext(sc_to_extcontext(sc));
return err ?: sig;
}
--
2.8.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] MIPS: Prevent "restoration" of MSA context in non-MSA kernels
2016-04-21 17:04 ` Paul Burton
(?)
@ 2016-05-11 17:17 ` Aaro Koskinen
-1 siblings, 0 replies; 7+ messages in thread
From: Aaro Koskinen @ 2016-05-11 17:17 UTC (permalink / raw)
To: Paul Burton, Ralf Baechle
Cc: linux-mips, Michal Toman, James Hogan, stable # v4 . 3+
Hi,
On Thu, Apr 21, 2016 at 06:04:53PM +0100, Paul Burton wrote:
> If a kernel doesn't support MSA context (ie. CONFIG_CPU_HAS_MSA=n) then
> it will only keep 64 bits per FP register in thread context, and the
> calls to set_fpr64 in restore_msa_extcontext will overrun the end of the
> FP register context into the FCSR & MSACSR values. GCC 6.x has become
> smart enough to detect this & complain like so:
>
> arch/mips/kernel/signal.c: In function 'protected_restore_fp_context':
> ./arch/mips/include/asm/processor.h:114:17: error: array subscript is above array bounds [-Werror=array-bounds]
> fpr->val##width[FPR_IDX(width, idx)] = val; \
> ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> ./arch/mips/include/asm/processor.h:118:1: note: in expansion of macro 'BUILD_FPR_ACCESS'
> BUILD_FPR_ACCESS(64)
>
> The only way to trigger this code to run would be for a program to set
> up an artificial extended MSA context structure following a sigframe &
> execute sigreturn. Whilst this doesn't allow a program to write to any
> state that it couldn't already, it makes little sense to allow this
> "restoration" of MSA context in a system that doesn't support MSA.
>
> Fix this by killing a program with SIGSYS if it tries something as crazy
> as "restoring" fake MSA context in this way, also fixing the build error
> & allowing for most of restore_msa_extcontext to be optimised out of
> kernels without support for MSA.
>
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Reported-by: Michal Toman <michal.toman@imgtec.com>
> Fixes: bf82cb30c7e5 ("MIPS: Save MSA extended context around signals")
> Cc: James Hogan <james.hogan@imgtec.com>
> Cc: stable <stable@vger.kernel.org> # v4.3+
This patch is needed to build MIPS kernel with GCC 6.1.
Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Thanks,
A.
> ---
>
> Changes in v2:
> - Prevent potential for malformed errno/signal from protected_restore_fp_context.
>
> arch/mips/kernel/signal.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
> index bf792e2..fc7c1f0 100644
> --- a/arch/mips/kernel/signal.c
> +++ b/arch/mips/kernel/signal.c
> @@ -195,6 +195,9 @@ static int restore_msa_extcontext(void __user *buf, unsigned int size)
> unsigned int csr;
> int i, err;
>
> + if (!config_enabled(CONFIG_CPU_HAS_MSA))
> + return SIGSYS;
> +
> if (size != sizeof(*msa))
> return -EINVAL;
>
> @@ -398,8 +401,8 @@ int protected_restore_fp_context(void __user *sc)
> }
>
> fp_done:
> - if (used & USED_EXTCONTEXT)
> - err |= restore_extcontext(sc_to_extcontext(sc));
> + if (!err && (used & USED_EXTCONTEXT))
> + err = restore_extcontext(sc_to_extcontext(sc));
>
> return err ?: sig;
> }
> --
> 2.8.0
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-05-11 17:27 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-21 15:39 [PATCH] MIPS: Prevent "restoration" of MSA context in non-MSA kernels Paul Burton
2016-04-21 15:39 ` Paul Burton
2016-04-21 16:41 ` Maciej W. Rozycki
2016-04-21 16:41 ` Maciej W. Rozycki
2016-04-21 17:04 ` [PATCH v2] " Paul Burton
2016-04-21 17:04 ` Paul Burton
2016-05-11 17:17 ` Aaro Koskinen
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.