linux-kernel - Re: [PATCH 2/2] ARM: copypage: do not use naked functions

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: <4e598f27e3dc7ae9fd96a6cf097d1154@agner.ch>
Date:   Tue, 16 Oct 2018 00:51:26 +0200
From:   Stefan Agner <stefan@...er.ch>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Nicolas Pitre <nicolas.pitre@...aro.org>,
        ulli.kroll@...glemail.com, joel@....id.au, arnd@...db.de,
        linus.walleij@...aro.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ARM: copypage: do not use naked functions

On 16.10.2018 00:41, Russell King - ARM Linux wrote:
> On Mon, Oct 15, 2018 at 06:35:33PM -0400, Nicolas Pitre wrote:
>> On Tue, 16 Oct 2018, Stefan Agner wrote:
>>
>> > GCC documentation says naked functions should only use basic ASM
>> > syntax. The extended ASM or mixture of basic ASM and "C" code is
>> > not guaranteed. Currently it seems to work though.
>> >
>> > Furthermore with Clang using parameters in extended asm in a
>> > naked function is not supported:
>> >   arch/arm/mm/copypage-v4wb.c:47:9: error: parameter references not
>> >   allowed in naked functions
>> >         : "r" (kto), "r" (kfrom), "I" (PAGE_SIZE / 64));
>> >                ^
>> >
>> > Use a regular function to be more portable. Also use volatile asm
>> > to avoid unsolicited optimizations.
>> >
>> > Tested with qemu versatileab machine and versatile_defconfig and
>> > qemu mainstone machine using pxa_defconfig compiled with GCC 7.2.1
>> > and Clang 7.0.
>> >
>> > Link: https://github.com/ClangBuiltLinux/linux/issues/90
>> > Reported-by: Joel Stanley <joel@....id.au>
>> > Signed-off-by: Stefan Agner <stefan@...er.ch>
>> > ---
>> >  arch/arm/mm/copypage-fa.c       | 17 +++++++++++------
>> >  arch/arm/mm/copypage-feroceon.c | 17 +++++++++++------
>> >  arch/arm/mm/copypage-v4mc.c     | 14 +++++++++-----
>> >  arch/arm/mm/copypage-v4wb.c     | 17 +++++++++++------
>> >  arch/arm/mm/copypage-v4wt.c     | 17 +++++++++++------
>> >  arch/arm/mm/copypage-xsc3.c     | 17 +++++++++++------
>> >  arch/arm/mm/copypage-xscale.c   | 13 ++++++++-----
>> >  7 files changed, 72 insertions(+), 40 deletions(-)
>> >
>> > diff --git a/arch/arm/mm/copypage-fa.c b/arch/arm/mm/copypage-fa.c
>> > index ec6501308c60..33ccd396bf99 100644
>> > --- a/arch/arm/mm/copypage-fa.c
>> > +++ b/arch/arm/mm/copypage-fa.c
>> > @@ -17,11 +17,16 @@
>> >  /*
>> >   * Faraday optimised copy_user_page
>> >   */
>> > -static void __naked
>> > -fa_copy_user_page(void *kto, const void *kfrom)
>> > +static void fa_copy_user_page(void *kto, const void *kfrom)
>> >  {
>> > -	asm("\
>> > -	stmfd	sp!, {r4, lr}			@ 2\n\
>> > +	register void *r0 asm("r0") = kto;
>> > +	register const void *r1 asm("r1") = kfrom;
>> > +
>> > +	asm(
>> > +	__asmeq("%0", "r0")
>> > +	__asmeq("%1", "r1")
>> > +	"\
>> > +	stmfd	sp!, {r4}			@ 2\n\
>> >  	mov	r2, %2				@ 1\n\
>> >  1:	ldmia	r1!, {r3, r4, ip, lr}		@ 4\n\
>> >  	stmia	r0, {r3, r4, ip, lr}		@ 4\n\
>> > @@ -34,9 +39,9 @@ fa_copy_user_page(void *kto, const void *kfrom)
>> >  	subs	r2, r2, #1			@ 1\n\
>> >  	bne	1b				@ 1\n\
>> >  	mcr	p15, 0, r2, c7, c10, 4		@ 1   drain WB\n\
>> > -	ldmfd	sp!, {r4, pc}			@ 3"
>> > +	ldmfd	sp!, {r4}			@ 3"
>> >  	:
>> > -	: "r" (kto), "r" (kfrom), "I" (PAGE_SIZE / 32));
>> > +	: "r" (r0), "r" (r1), "I" (PAGE_SIZE / 32));
>>
>> This is still wrong as you list r0 and r1 in the input operand list
>> where they must remain constant but the code does modify them. You
>> should list them in the output operand list with the "&" attribute. Also
>> r2 should be listed in the clobbered list.
> 
> Either we keep these as naked functions (and, if Clang wants to
> try to inline naked functions which makes no sense, also mark them
> as noinline) or we make them proper functions and also add (eg) r4
> to the clobber list and get rid of the stacking of that register
> along with LR/PC.

Clang does not inline naked functions, at least that is what a quick
look at the disassembled code shows when compiling with 9a40ac86152c
reverted.

> 
> Having this half-way house which will generate worse code is not
> acceptable.

For Clang reverting 9a40ac86152c ("ARM: 6164/1: Add kto and kfrom to
input operands list.") is a solution...

I guess the question is why that commit was necessary back then... Do we
break something by reverting it?

--
Stefan