linux-kernel - Re: [PATCH] arm64: Delete the space separator in __emit

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]

Date:   Tue, 14 Apr 2020 08:43:07 -0700
From:   Fangrui Song <maskray@...gle.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        clang-built-linux@...glegroups.com
Subject: Re: [PATCH] arm64: Delete the space separator in __emit_inst


On 2020-04-14, Mark Rutland wrote:
>Hi Fangrui,
>
>On Sun, Apr 12, 2020 at 08:38:11PM -0700, Fangrui Song wrote:
>> Many instances of __emit_inst(x) expand to a directive. In a few places
>> it is used as a macro argument, e.g.
>>
>>   arch/arm64/include/asm/sysreg.h
>>   #define __emit_inst(x)                       .inst (x)
>>
>>   arch/arm64/include/asm/sysreg.h
>>   #define SET_PSTATE_PAN(x)            __emit_inst(0xd500401f | PSTATE_PAN | ((!!x) << PSTATE_Imm_shift))
>>
>>   arch/arm64/kvm/hyp/entry.S
>>   ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
>>
>> Clang integrated assembler parses `.inst (x)` as two arguments passing
>> to a macro. We delete the space separator so that `.inst(x)` will be
>> parsed as one argument.
>
>I'm a little confused by the above; sorry if the below sounds stupid or
>pedantic, but I just want to make sure I've understood the problem
>correctly.
>
>For the above, ALTERNATIVE() and SET_PSTATE_PAN() are both preprocessor
>macros, so I would expect those to be expanded before either the
>integrated assembler or an external assembler consumes any of the
>assembly (and both would see the same expanded text). Given that, I'm a
>bit confused as to why the integrated assembly would have an impact on
>preprocessing.
>
>Does compiling the pre-processed source using the integrated assembler
>result in the same behaviour? Can we see the expanded text to make that
>clear?
>
>... at what stage exactly does this go wrong?
>
>Thanks,
>Mark.

Hi Mark,

The C preprocessor expands arch/arm64/kvm/hyp/entry.S
    ALTERNATIVE(nop, SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)

to:
    alternative_insn nop, .inst (0xd500401f | ((0) << 16 | (4) << 5) | ((!!1) << 8)), 4, 1

`alternative_insn` is an assembler macro, not handled by the C preprocessor.

Both comma and space are separators, with an exception that content
inside a pair of parentheses/quotes is not split, so clang -cc1as or GNU
as x86 splits arguments this way:

    nop, .inst, (0xd500401f | ((0) << 16 | (4) << 5) | ((!!1) << 8)), 4, 1

I actually feel that GNU as arm64's behavior is a little bit buggy. It
works just because GNU as has another preprocessing step `do_scrub_chars`
and its arm64 backend deletes the space before '('

    alternative_insn nop,.inst(0xd500401f|((0)<<16|(4)<<5)|((!!1)<<8)),4,1

The x86 backend keeps the space before the outmost '('

   alternative_insn nop,.inst (0xd500401f|((0)<<16|(4)<<5)|((!!1)<<8)),4,1

By reading its state machine, I think keeping the spaces will be the
most reasonable behavior.

If .inst were only used as arguments,

    alternative_insn nop, ".inst (0xd500401f | ((0) << 16 | (4) << 5) | ((!!1) << 8))", 4, 1

would be the best to avoid parsing issues.

>>
>> Note, GNU as parsing `.inst (x)` as one argument is unintentional (for
>> example the x86 backend will parse the construct as two arguments).
>> See https://sourceware.org/bugzilla/show_bug.cgi?id=25750#c10
>>
>> Link: https://github.com/ClangBuiltLinux/linux/issues/939
>> Cc: clang-built-linux@...glegroups.com
>> Signed-off-by: Fangrui Song <maskray@...gle.com>
>> ---
>>  arch/arm64/include/asm/sysreg.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index ebc622432831..af21e2ec5e3e 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -49,7 +49,9 @@
>>  #ifndef CONFIG_BROKEN_GAS_INST
>>
>>  #ifdef __ASSEMBLY__
>> -#define __emit_inst(x)			.inst (x)
>> +// The space separator is omitted so that __emit_inst(x) can be parsed as
>> +// either a directive or a macro argument.
>> +#define __emit_inst(x)			.inst(x)
>>  #else
>>  #define __emit_inst(x)			".inst " __stringify((x)) "\n\t"
>>  #endif
>> --
>> 2.26.0.110.g2183baf09c-goog
>>