[PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against the ABI of the task

[PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against the ABI of the task

[Maciej W. Rozycki]

Fix an API loophole introduced with commit 9791554b45a2 ("MIPS,prctl: 
add PR_[GS]ET_FP_MODE prctl options for MIPS"), where the caller of 
prctl(2) is incorrectly allowed to make a change to CP0.Status.FR or 
CP0.Config5.FRE register bits even if CONFIG_MIPS_O32_FP64_SUPPORT has 
not been enabled, despite that an executable requesting the mode 
requested via ELF file annotation would not be allowed to run in the 
first place, or for n64 and n64 ABI tasks which do not have non-default 
modes defined at all.  Add suitable checks to `mips_set_process_fp_mode' 
and bail out if an invalid mode change has been requested for the ABI in 
effect, even if the FPU hardware or emulation would otherwise allow it.

Always succeed however without taking any further action if the mode 
requested is the same as one already in effect, regardless of whether 
any mode change, should it be requested, would actually be allowed for 
the task concerned.

Cc: stable@vger.kernel.org # 4.0+
Fixes: 9791554b45a2 ("MIPS,prctl: add PR_[GS]ET_FP_MODE prctl options for MIPS")
Signed-off-by: Maciej W. Rozycki <macro@mips.com>
---
 arch/mips/kernel/process.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

linux-mips-prctl-fp-mode-o32-fp64.diff
Index: linux-sfr-test/arch/mips/kernel/process.c
===================================================================
--- linux-sfr-test.orig/arch/mips/kernel/process.c	2017-11-25 12:40:55.868109000 +0000
+++ linux-sfr-test/arch/mips/kernel/process.c	2017-11-25 12:41:56.411578000 +0000
@@ -705,6 +705,18 @@ int mips_set_process_fp_mode(struct task
 	struct task_struct *t;
 	int max_users;
 
+	/* If nothing to change, return right away, successfully.  */
+	if (value == mips_get_process_fp_mode(task))
+		return 0;
+
+	/* Only accept a mode change if 64-bit FP enabled for o32.  */
+	if (!IS_ENABLED(CONFIG_MIPS_O32_FP64_SUPPORT))
+		return -EOPNOTSUPP;
+
+	/* And only for o32 tasks.  */
+	if (IS_ENABLED(CONFIG_64BIT) && !test_thread_flag(TIF_32BIT_REGS))
+		return -EOPNOTSUPP;
+
 	/* Check the value is valid */
 	if (value & ~known_bits)
 		return -EOPNOTSUPP;

Re: [PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against the ABI of the task

[Paul Burton]

Hi Maciej,

On Mon, Nov 27, 2017 at 09:33:03AM +0000, Maciej W. Rozycki wrote:
> Fix an API loophole introduced with commit 9791554b45a2 ("MIPS,prctl: 
> add PR_[GS]ET_FP_MODE prctl options for MIPS"), where the caller of 
> prctl(2) is incorrectly allowed to make a change to CP0.Status.FR or 
> CP0.Config5.FRE register bits even if CONFIG_MIPS_O32_FP64_SUPPORT has 
> not been enabled, despite that an executable requesting the mode 
> requested via ELF file annotation would not be allowed to run in the 
> first place, or for n64 and n64 ABI tasks which do not have non-default 
> modes defined at all.  Add suitable checks to `mips_set_process_fp_mode' 
> and bail out if an invalid mode change has been requested for the ABI in 
> effect, even if the FPU hardware or emulation would otherwise allow it.

This seems reasonable, though in my view more because the FPU emulator
optimises out code for cases we shouldn't hit via cop1_64bit(). Allowing
user code to trigger these cases can only lead to odd and incorrect
behaviour so preventing that makes sense.

> Always succeed however without taking any further action if the mode 
> requested is the same as one already in effect, regardless of whether 
> any mode change, should it be requested, would actually be allowed for 
> the task concerned.

This seems like a distinct change that I think would be worth splitting
out to a separate patch.

Both changes look good to me though, so feel free to add:

    Reviewed-by: Paul Burton <paul.burton@mips.com>

Thanks,
    Paul

> Cc: stable@vger.kernel.org # 4.0+
> Fixes: 9791554b45a2 ("MIPS,prctl: add PR_[GS]ET_FP_MODE prctl options for MIPS")
> Signed-off-by: Maciej W. Rozycki <macro@mips.com>
> ---
>  arch/mips/kernel/process.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> linux-mips-prctl-fp-mode-o32-fp64.diff
> Index: linux-sfr-test/arch/mips/kernel/process.c
> ===================================================================
> --- linux-sfr-test.orig/arch/mips/kernel/process.c	2017-11-25 12:40:55.868109000 +0000
> +++ linux-sfr-test/arch/mips/kernel/process.c	2017-11-25 12:41:56.411578000 +0000
> @@ -705,6 +705,18 @@ int mips_set_process_fp_mode(struct task
>  	struct task_struct *t;
>  	int max_users;
>  
> +	/* If nothing to change, return right away, successfully.  */
> +	if (value == mips_get_process_fp_mode(task))
> +		return 0;
> +
> +	/* Only accept a mode change if 64-bit FP enabled for o32.  */
> +	if (!IS_ENABLED(CONFIG_MIPS_O32_FP64_SUPPORT))
> +		return -EOPNOTSUPP;
> +
> +	/* And only for o32 tasks.  */
> +	if (IS_ENABLED(CONFIG_64BIT) && !test_thread_flag(TIF_32BIT_REGS))
> +		return -EOPNOTSUPP;
> +
>  	/* Check the value is valid */
>  	if (value & ~known_bits)
>  		return -EOPNOTSUPP;

Re: [PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against the ABI of the task

[Maciej W. Rozycki]

Hi Paul,

> > Fix an API loophole introduced with commit 9791554b45a2 ("MIPS,prctl: 
> > add PR_[GS]ET_FP_MODE prctl options for MIPS"), where the caller of 
> > prctl(2) is incorrectly allowed to make a change to CP0.Status.FR or 
> > CP0.Config5.FRE register bits even if CONFIG_MIPS_O32_FP64_SUPPORT has 
> > not been enabled, despite that an executable requesting the mode 
> > requested via ELF file annotation would not be allowed to run in the 
> > first place, or for n64 and n64 ABI tasks which do not have non-default 
> > modes defined at all.  Add suitable checks to `mips_set_process_fp_mode' 
> > and bail out if an invalid mode change has been requested for the ABI in 
> > effect, even if the FPU hardware or emulation would otherwise allow it.
> 
> This seems reasonable, though in my view more because the FPU emulator
> optimises out code for cases we shouldn't hit via cop1_64bit(). Allowing
> user code to trigger these cases can only lead to odd and incorrect
> behaviour so preventing that makes sense.

 For !CONFIG_MIPS_O32_FP64_SUPPORT we end up with messed up state, yes. 
However for n64/n32 (and CONFIG_MIPS_O32_FP64_SUPPORT set) the state 
should be consistent yet not defined by the respective ABIs, so we 
shouldn't allow that either.

> > Always succeed however without taking any further action if the mode 
> > requested is the same as one already in effect, regardless of whether 
> > any mode change, should it be requested, would actually be allowed for 
> > the task concerned.
> 
> This seems like a distinct change that I think would be worth splitting
> out to a separate patch.

 I've been thinking about it before posting and decided it's inherent.  

 Indeed in developing this fix this part was the last one I realised that 
had to be done for the change to be overall self-consistent, following a 
principle typically applied to hardware registers where the programmer is 
architecturally allowed to write individual bits with the values 
previously read from them even if these bits are undefined in the 
specification of hardware concerned.

 So here you'll be able to issue a PR_SET_FP_MODE request with a value 
previously obtained with PR_GET_FP_MODE and it will succeed, even if all 
the bits are actually read-only for the ABI in effect.  This is important 
as GDB will soon be using these calls and expect PR_SET_FP_MODE not to 
fail if an attempt is made to write back a value previously obtained with 
PR_GET_FP_MODE.

 I could have buried this check in the two conditions that follow, making 
execution fall through if the mode remains unchanged, however I have 
realised that making the check upfront makes the resulting code cleaner.

 That written, I could make it 1/2 with the ABI checks becoming 2/2, but 
then 1/2 wouldn't make sense on its own (except perhaps as a 
microoptimisation, but that would be an entirely different purpose) and 
would have to be considered in conjunction with 2/2 anyway.

> Both changes look good to me though, so feel free to add:
> 
>     Reviewed-by: Paul Burton <paul.burton@mips.com>

 Thanks for your review.  Do you feel convinced with the justification I 
gave?

  Maciej

Re: [PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against the ABI of the task

[Paul Burton]

Hi Maciej,

On Mon, Nov 27, 2017 at 09:39:10PM +0000, Maciej W. Rozycki wrote:
> > > Always succeed however without taking any further action if the mode 
> > > requested is the same as one already in effect, regardless of whether 
> > > any mode change, should it be requested, would actually be allowed for 
> > > the task concerned.
> > 
> > This seems like a distinct change that I think would be worth splitting
> > out to a separate patch.
> 
>  I've been thinking about it before posting and decided it's inherent.  
> 
>  Indeed in developing this fix this part was the last one I realised that 
> had to be done for the change to be overall self-consistent, following a 
> principle typically applied to hardware registers where the programmer is 
> architecturally allowed to write individual bits with the values 
> previously read from them even if these bits are undefined in the 
> specification of hardware concerned.
> 
>  So here you'll be able to issue a PR_SET_FP_MODE request with a value 
> previously obtained with PR_GET_FP_MODE and it will succeed, even if all 
> the bits are actually read-only for the ABI in effect.  This is important 
> as GDB will soon be using these calls and expect PR_SET_FP_MODE not to 
> fail if an attempt is made to write back a value previously obtained with 
> PR_GET_FP_MODE.
> 
>  I could have buried this check in the two conditions that follow, making 
> execution fall through if the mode remains unchanged, however I have 
> realised that making the check upfront makes the resulting code cleaner.
> 
>  That written, I could make it 1/2 with the ABI checks becoming 2/2, but 
> then 1/2 wouldn't make sense on its own (except perhaps as a 
> microoptimisation, but that would be an entirely different purpose) and 
> would have to be considered in conjunction with 2/2 anyway.

Ah - OK, I see. Prior to this patch the value returned by PR_GET_FP_MODE
would always be one accepted by PR_SET_FP_MODE anyway, but with the
patch that will cease to be true for non-o32 ABIs without the special
case. Gotcha.

> > Both changes look good to me though, so feel free to add:
> > 
> >     Reviewed-by: Paul Burton <paul.burton@mips.com>
> 
>  Thanks for your review.  Do you feel convinced with the justification I 
> gave?

Yes - I follow, please consider the Reviewed-by tag valid for the patch
as-is.

Thanks,
    Paul

Re: [PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against the ABI of the task

[Maciej W. Rozycki]

Hi Paul,

> >  That written, I could make it 1/2 with the ABI checks becoming 2/2, but 
> > then 1/2 wouldn't make sense on its own (except perhaps as a 
> > microoptimisation, but that would be an entirely different purpose) and 
> > would have to be considered in conjunction with 2/2 anyway.
> 
> Ah - OK, I see. Prior to this patch the value returned by PR_GET_FP_MODE
> would always be one accepted by PR_SET_FP_MODE anyway, but with the
> patch that will cease to be true for non-o32 ABIs without the special
> case. Gotcha.

 And also for !CONFIG_MIPS_O32_FP64_SUPPORT.

> > > Both changes look good to me though, so feel free to add:
> > > 
> > >     Reviewed-by: Paul Burton <paul.burton@mips.com>
> > 
> >  Thanks for your review.  Do you feel convinced with the justification I 
> > gave?
> 
> Yes - I follow, please consider the Reviewed-by tag valid for the patch
> as-is.

 Great!  Thanks again for your review.

  Maciej