linux-kernel - Re: [PATCH 1/2] ARM: copypage-fa: add kto and kfrom to input operands list

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: <26d465580722c7f65b6916e96e283967@agner.ch>
Date:   Tue, 16 Oct 2018 00:39:54 +0200
From:   Stefan Agner <stefan@...er.ch>
To:     Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     raj.khem@...il.com, ulli.kroll@...glemail.com, joel@....id.au,
        nico@...aro.org, arnd@...db.de, linus.walleij@...aro.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] ARM: copypage-fa: add kto and kfrom to input operands
 list

On 16.10.2018 00:23, Russell King - ARM Linux wrote:
> On Tue, Oct 16, 2018 at 12:16:29AM +0200, Stefan Agner wrote:
>> When functions incoming parameters are not in input operands list gcc
>> 4.5 does not load the parameters into registers before calling this
>> function but the inline assembly assumes valid addresses inside this
>> function. This breaks the code because r0 and r1 are invalid when
>> execution enters v4wb_copy_user_page ()
> 
> NAK.  Naked functions must never be inlined.  Please add a "noinline"
> attribute to the function rather than making things more complex.
> 

To be honest, I did not put much thought into this commit since it is
just doing to copypage-fa.c what 9a40ac86152c ("ARM: 6164/1: Add kto and
kfrom to input operands list.") has been done to the other copypage
implementations...

[adding Khem]

> The GCC manual states:
> 
> `naked'
>      Use this attribute on the ARM, AVR, MCORE, MSP430, NDS32, RL78, RX
>      and SPU ports to indicate that the specified function does not
>      need prologue/epilogue sequences generated by the compiler.  It is
>      up to the programmer to provide these sequences. The only
>                                                       ^^^^^^^^
>      statements that can be safely included in naked functions are
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      `asm' statements that do not have operands.  All other statements,
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      including declarations of local variables, `if' statements, and so
>      forth, should be avoided.  Naked functions should be used to
>      implement the body of an assembly function, while allowing the
>      compiler to construct the requisite function declaration for the
>      assembler.
> 
> The 'I' attribute is fine here because it is a constant that is not
> allowed to be in a register (and hence has no code generation side
> effects.)
> 
> Adding operands for the input parameters, however, isn't going to
> work around the fact that _this_ assembly is written to be out of
> line and so it must never be inlined by the compiler.

I briefly looked at a disassembled version after applying both patches,
it indeed leads to inlining. However, the code seems to be working
(thanks to asm volatile?)...

Anyway, my goal is actually what patch 2 ("ARM: copypage: do not use
naked functions") is doing: Make Clang happy. As a matter of fact,
reverting 9a40ac86152c actually fixes compilation for Clang too, and
seems to lead to a working Kernel (tested with versatile_defconfig in
Qemu), so maybe that is what we should do here?

--
Stefan

> 
>> Also the constant needs to be used as third input operand so account
>> for that as well.
>>
>> This fixes copypage-fa.c what has previously done before for the other
>> copypage implementations in commit 9a40ac86152c ("ARM: 6164/1: Add kto
>> and kfrom to input operands list.").
>>
>> Signed-off-by: Stefan Agner <stefan@...er.ch>
>> ---
>>  arch/arm/mm/copypage-fa.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mm/copypage-fa.c b/arch/arm/mm/copypage-fa.c
>> index d130a5ece5d5..ec6501308c60 100644
>> --- a/arch/arm/mm/copypage-fa.c
>> +++ b/arch/arm/mm/copypage-fa.c
>> @@ -22,7 +22,7 @@ fa_copy_user_page(void *kto, const void *kfrom)
>>  {
>>  	asm("\
>>  	stmfd	sp!, {r4, lr}			@ 2\n\
>> -	mov	r2, %0				@ 1\n\
>> +	mov	r2, %2				@ 1\n\
>>  1:	ldmia	r1!, {r3, r4, ip, lr}		@ 4\n\
>>  	stmia	r0, {r3, r4, ip, lr}		@ 4\n\
>>  	mcr	p15, 0, r0, c7, c14, 1		@ 1   clean and invalidate D line\n\
>> @@ -36,7 +36,7 @@ fa_copy_user_page(void *kto, const void *kfrom)
>>  	mcr	p15, 0, r2, c7, c10, 4		@ 1   drain WB\n\
>>  	ldmfd	sp!, {r4, pc}			@ 3"
>>  	:
>> -	: "I" (PAGE_SIZE / 32));
>> +	: "r" (kto), "r" (kfrom), "I" (PAGE_SIZE / 32));
>>  }
>>
>>  void fa_copy_user_highpage(struct page *to, struct page *from,
>> --
>> 2.19.1
>>