linux-kernel - Re: [RFC patch 07/15] arm64/syscall: Remove obscure flag check

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: <20190920142920.GB21231@arrakis.emea.arm.com>
Date:   Fri, 20 Sep 2019 15:29:20 +0100
From:   Catalin Marinas <catalin.marinas@....com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Marc Zyngier <maz@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-arch@...r.kernel.org
Subject: Re: [RFC patch 07/15] arm64/syscall: Remove obscure flag check

On Thu, Sep 19, 2019 at 05:03:21PM +0200, Thomas Gleixner wrote:
> The syscall handling code has an obscure check of pending work which does a
> shortcut before returning to user space. It calls into the exit work code
> when the flags at entry time required an entry into the slowpath. That does
> not make sense because the underlying work functionality will reevaluate
> the flags anyway and not do anything.

The current C code was just matching the original behaviour in asm
(converted by commit f37099b6992a0b81). The idea IIRC was to always pair
a syscall_trace_enter() with a syscall_trace_exit() irrespective of the
thread flag changes. I think the behaviour is preserved with your patch
if no-one clears the work flags during el0_svc_common().

> @@ -105,33 +103,15 @@ static void el0_svc_common(struct pt_reg
>  	user_exit();
>  
>  	scno = syscall_enter_from_usermode(regs, scno);
> -	if (scno == NO_SYSCALL)
> -		goto trace_exit;
> -
> -	invoke_syscall(regs, scno, sc_nr, syscall_table);
> +	if (scno != NO_SYSCALL)
> +		invoke_syscall(regs, scno, sc_nr, syscall_table);
>  
> -	/*
> -	 * The tracing status may have changed under our feet, so we have to
> -	 * check again. However, if we were tracing entry, then we always trace
> -	 * exit regardless, as the old entry assembly did.
> -	 */
> -	if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
> -		local_daif_mask();
> -		flags = current_thread_info()->flags;
> -		if (!has_syscall_work(flags)) {
> -			/*
> -			 * We're off to userspace, where interrupts are
> -			 * always enabled after we restore the flags from
> -			 * the SPSR.
> -			 */
> -			trace_hardirqs_on();
> -			return;
> -		}
> +	local_daif_mask();
> +	if (has_syscall_work(current_thread_info()->flags) ||
> +	    IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
>  		local_daif_restore(DAIF_PROCCTX);
> +		syscall_trace_exit(regs);
>  	}

That's missing a trace_hardirqs_on() (off done in local_daif_mask())
before returning.

> -
> -trace_exit:
> -	syscall_trace_exit(regs);
>  }

-- 
Catalin