On 5 October 2017 at 09:38, Borislav Petkov <b...@alien8.de> wrote:
> On Wed, Oct 04, 2017 at 08:08:12PM +0200, Mathias Krause wrote:
>> The alt_max_short() macro in asm/alternative.h does not work as
>> intended, leading to nasty bugs. E.g. alt_max_short("1", "3")
>> evaluates to 3, but alt_max_short("3", "1") evaluates to 1 -- not
>> exactly the maximum of 1 and 3.
>>
>> In fact, I had to learn it the hard way by crashing my kernel in not
>> so funny ways by attempting to make use of the ALTENATIVE_2 macro
>> with alternatives where the first one was larger than the second
>> one.
>>
>> According to [1] and commit dbe4058a6a44 ("x86/alternatives: Fix
>> ALTERNATIVE_2 padding generation properly") the right handed side
>> should read "-(-(a < b))" not "-(-(a - b))". Fix that, to make the
>> macro work as intended.
>>
>> While at it, fix up the comment regarding the additional "-", too.
>> It's not about gas' usage of s32 but brain dead logic of having a
>> "true" value of -1 for the < operator ... *sigh*
>>
>> Btw., the one in asm/alternative-asm.h is correct. And, apparently,
>> all current users of ALTERNATIVE_2() pass same sized alternatives,
>> avoiding to hit the bug.
>>
>> [1] http://graphics.stanford.edu/~seander/bithacks.html#IntegerMinOrMax
>>
>> Fixes: dbe4058a6a44 ("x86/alternatives: Fix ALTERNATIVE_2 padding generation
>> properly")
>> Signed-off-by: Mathias Krause <mini...@googlemail.com>
>> Cc: Borislav Petkov <b...@suse.de>
>> ---
>> arch/x86/include/asm/alternative.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/alternative.h
>> b/arch/x86/include/asm/alternative.h
>> index c096624137ae..7c553f48f163 100644
>> --- a/arch/x86/include/asm/alternative.h
>> +++ b/arch/x86/include/asm/alternative.h
>> @@ -106,9 +106,9 @@ static inline int alternatives_text_reserved(void
>> *start, void *end)
>> * max without conditionals. Idea adapted from:
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> You did read this part, right?
Yes. But it's just wrong the way it is right now. It's a crap number
generator instead of being a "max without conditionals".
> AFAIR, gas can't stomach conditionals but I don't remember the details
> anymore.
Can't be as arch/x86/include/asm/alternative.h itself makes use of
them for the implementation of ALTERNATIVE[_2], see, e.g. __OLDINSTR()
and OLDINSTR_2().
> Could be that -1 representation of "true". Let me add Micha to
> CC.
>
> Anyway, how can I reproduce your observation? Code snippet and compiler
> pls.
Here you go:
$ cat alt_max.c
#ifdef VANILLA
# define alt_max_short(a, b) \
"((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") - (" b ")))))"
#else
# define alt_max_short(a, b) \
"((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
#endif
#define ams(a, b) \
"ams_" #a "_" #b " = " alt_max_short(#a, #b) "\n\t"
asm(ams(1, 3)
ams(3, 1)
ams(1, 6));
$ gcc -DVANILLA -c alt_max.c && nm alt_max.o
0000000000000003 a ams_1_3
0000000000000002 a ams_1_6
0000000000000001 a ams_3_1
$ gcc -c alt_max.c && nm alt_max.o
0000000000000003 a ams_1_3
0000000000000006 a ams_1_6
0000000000000003 a ams_3_1
Note not only how it gets the max(3,1) case wrong but generates even
more insane crap for max(1,6).
$ as --version | head -1
GNU assembler (GNU Binutils for Debian) 2.25
Cheers,
Mathias
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
>
> Good mailing practices for 400: avoid top-posting and trim the reply.
