linux-kernel - Re: [PATCH] MIPS: Fix MSA ld unaligned failure cases

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] [day] [month] [year] [list]

Message-ID: <20160203144417.GI5038@jhogan-linux.le.imgtec.org>
Date:	Wed, 3 Feb 2016 14:44:17 +0000
From:	James Hogan <james.hogan@...tec.com>
To:	Paul Burton <paul.burton@...tec.com>
CC:	<linux-mips@...ux-mips.org>, Ralf Baechle <ralf@...ux-mips.org>,
	"stable # v4 . 3" <stable@...r.kernel.org>,
	Leonid Yegoshin <Leonid.Yegoshin@...tec.com>,
	"Maciej W. Rozycki" <macro@...ux-mips.org>,
	<linux-kernel@...r.kernel.org>,
	James Cowgill <James.Cowgill@...tec.com>,
	Markos Chandras <markos.chandras@...tec.com>
Subject: Re: [PATCH] MIPS: Fix MSA ld unaligned failure cases

On Wed, Feb 03, 2016 at 03:35:49AM +0000, Paul Burton wrote:
> Copying the content of an MSA vector from user memory may involve TLB
> faults & mapping in pages. This will fail when preemption is disabled
> due to an inability to acquire mmap_sem from do_page_fault, which meant
> such vector loads to unmapped pages would always fail to be emulated.
> Fix this by disabling preemption later only around the updating of
> vector register state.
> 
> This change does however introduce a race between performing the load
> into thread context & the thread being preempted, saving its current
> live context & clobbering the loaded value. This should be a rare
> occureence, so optimise for the fast path by simply repeating the load if
> we are preempted.
> 
> Additionally if the copy failed then the failure path was taken with
> preemption left disabled, leading to the kernel typically encountering
> further issues around sleeping whilst atomic. The change to where
> preemption is disabled avoids this issue.
> 
> Fixes: e4aa1f153add "MIPS: MSA unaligned memory access support"
> Reported-by: James Hogan <james.hogan@...tec.com>
> Signed-off-by: Paul Burton <paul.burton@...tec.com>
> Cc: stable <stable@...r.kernel.org> # v4.3

Reviewed-by: James Hogan <james.hogan@...tec.com>

Cheers
James

> 
> ---
> 
>  arch/mips/kernel/unaligned.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/mips/kernel/unaligned.c b/arch/mips/kernel/unaligned.c
> index 490cea5..5c62065 100644
> --- a/arch/mips/kernel/unaligned.c
> +++ b/arch/mips/kernel/unaligned.c
> @@ -885,7 +885,7 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>  {
>  	union mips_instruction insn;
>  	unsigned long value;
> -	unsigned int res;
> +	unsigned int res, preempted;
>  	unsigned long origpc;
>  	unsigned long orig31;
>  	void __user *fault_addr = NULL;
> @@ -1226,27 +1226,36 @@ static void emulate_load_store_insn(struct pt_regs *regs,
>  			if (!access_ok(VERIFY_READ, addr, sizeof(*fpr)))
>  				goto sigbus;
>  
> -			/*
> -			 * Disable preemption to avoid a race between copying
> -			 * state from userland, migrating to another CPU and
> -			 * updating the hardware vector register below.
> -			 */
> -			preempt_disable();
> -
> -			res = __copy_from_user_inatomic(fpr, addr,
> -							sizeof(*fpr));
> -			if (res)
> -				goto fault;
> -
> -			/*
> -			 * Update the hardware register if it is in use by the
> -			 * task in this quantum, in order to avoid having to
> -			 * save & restore the whole vector context.
> -			 */
> -			if (test_thread_flag(TIF_USEDMSA))
> -				write_msa_wr(wd, fpr, df);
> +			do {
> +				/*
> +				 * If we have live MSA context keep track of
> +				 * whether we get preempted in order to avoid
> +				 * the register context we load being clobbered
> +				 * by the live context as it's saved during
> +				 * preemption. If we don't have live context
> +				 * then it can't be saved to clobber the value
> +				 * we load.
> +				 */
> +				preempted = test_thread_flag(TIF_USEDMSA);
> +
> +				res = __copy_from_user_inatomic(fpr, addr,
> +								sizeof(*fpr));
> +				if (res)
> +					goto fault;
>  
> -			preempt_enable();
> +				/*
> +				 * Update the hardware register if it is in use
> +				 * by the task in this quantum, in order to
> +				 * avoid having to save & restore the whole
> +				 * vector context.
> +				 */
> +				preempt_disable();
> +				if (test_thread_flag(TIF_USEDMSA)) {
> +					write_msa_wr(wd, fpr, df);
> +					preempted = 0;
> +				}
> +				preempt_enable();
> +			} while (preempted);
>  			break;
>  
>  		case msa_st_op:
> -- 
> 2.7.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)