linux-kernel - Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread

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: <20200518133750.GF32394@willie-the-truck>
Date:   Mon, 18 May 2020 14:37:50 +0100
From:   Will Deacon <will@...nel.org>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Sami Tolvanen <samitolvanen@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Mark Rutland <mark.rutland@...com>,
        Jann Horn <jannh@...gle.com>, Ard Biesheuvel <ardb@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>, kernel-team@...roid.com
Subject: Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value
 in thread_info

On Mon, May 18, 2020 at 12:37:10PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:51PM +0100, Will Deacon wrote:
> > Storing the SCS information in thread_info as a {base,offset} pair
> > introduces an additional load instruction on the ret-to-user path,
> > since the SCS stack pointer in x18 has to be converted back to an offset
> > by subtracting the base.
> > 
> > Replace the offset with the absolute SCS stack pointer value instead
> > and avoid the redundant load.
> > 
> > Signed-off-by: Will Deacon <will@...nel.org>
> 
> One trivial nit below, but regardless this looks sound to me, and I
> certainly prefer having the absolute address rather than an offset, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@....com>
> 
> > diff --git a/kernel/scs.c b/kernel/scs.c
> > index 9389c28f0853..5ff8663e4a67 100644
> > --- a/kernel/scs.c
> > +++ b/kernel/scs.c
> > @@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
> >  	if (!s)
> >  		return -ENOMEM;
> >  
> > -	task_scs(tsk) = s;
> > -	task_scs_offset(tsk) = 0;
> > +	task_scs(tsk) = task_scs_sp(tsk) = s;
> 
> I think this would be more legible as two statements:
> 
> |	task_sys(tsk) = s;
> |	task_scs_sp(tsk) = s;

I think it's nice to be able to say:

	task_scs(tsk) = task_scs_sp(tsk);

because it makes it very clear that they are initialised to the same thing.
Having it as two statements means somebody will update one and forget to
update the other one.

> ... as we usually save `foo = bar = baz` stuff for the start of a
> function or within loop conditions.

Hmm, I can't really find anything consistent in that regard, to be honest
with you. Did I miss something in the coding style doc?

I'll leave it as-is for now.

Will