linux-kernel - Re: [PATCH] x86/alternatives: Fix alt_max

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: <CA+rthh_EAOQ-VyoJ1zR=16n1_T0-wJPG7N1ESDBirzY7AbBXAA@mail.gmail.com>
Date:   Thu, 5 Oct 2017 09:58:32 +0200
From:   Mathias Krause <minipli@...glemail.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...e.de>,
        x86-ml <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Michael Matz <matz@...e.de>
Subject: Re: [PATCH] x86/alternatives: Fix alt_max_short macro to really be a max()

On 5 October 2017 at 09:38, Borislav Petkov <bp@...en8.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 <minipli@...glemail.com>
>> Cc: Borislav Petkov <bp@...e.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.