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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171127184642.ny2lad4y6zz6am2b@pburton-laptop>
Date:   Mon, 27 Nov 2017 10:46:42 -0800
From:   Paul Burton <paul.burton@...s.com>
To:     "Maciej W. Rozycki" <macro@...s.com>
CC:     Ralf Baechle <ralf@...ux-mips.org>,
        James Hogan <james.hogan@...s.com>,
        <linux-mips@...ux-mips.org>, <linux-kernel@...r.kernel.org>,
        <stable@...r.kernel.org>
Subject: Re: [PATCH] MIPS: Validate PR_SET_FP_MODE prctl(2) requests against
 the ABI of the task

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@...s.com>

Thanks,
    Paul

> Cc: stable@...r.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@...s.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;