linux-kernel - Re: AMD DC graphics display code enables -mhard-float, -msse, -msse2 without any visible FPU state protection

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: <56fd0509-ebef-f7b5-3ca1-fc51ca493a4c@amd.com>
Date:   Fri, 10 Apr 2020 16:31:39 +0200
From:   Christian König <christian.koenig@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Jann Horn <jannh@...gle.com>,
        Harry Wentland <harry.wentland@....com>,
        Leo Li <sunpeng.li@....com>, amd-gfx@...ts.freedesktop.org,
        Alex Deucher <alexander.deucher@....com>,
        "David (ChunMing) Zhou" <David1.Zhou@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        the arch/x86 maintainers <x86@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        mhiramat@...nel.org
Subject: Re: AMD DC graphics display code enables -mhard-float, -msse, -msse2
 without any visible FPU state protection

Am 09.04.20 um 22:01 schrieb Peter Zijlstra:
> On Thu, Apr 09, 2020 at 08:15:57PM +0200, Christian König wrote:
>> Am 09.04.20 um 19:09 schrieb Peter Zijlstra:
>>> On Thu, Apr 09, 2020 at 05:59:56PM +0200, Peter Zijlstra wrote:
>>> [SNIP]
>>>> I'll need another approach, let me consider.
>>> Christian; it says these files are generated, does that generator know
>>> which functions are wholly in FPU context and which are not?
>> Well that "generator" is still a human being :)
>>
>> It's just that the formulae for the calculation come from the hardware team
>> and we are not able to easily transcript them to fixed point calculations.
> Well, if it's a human, can this human respect the kernel coding style a
> bit more :-) Some of that stuff is atrocious.

Yes, I know. That's unfortunately something we still need to work on as 
well.

>> We are currently in the process of moving all the stuff which requires
>> floating point into a single C file(s) and then make sure that we only call
>> those within kernel_fpu_begin()/end() blocks.
> Can you make the build system stick all those .o files in a single
> archive? That's the only way I can do call validation; external
> relocatoin records do not contain the section.

Need to double check that with the display team responsible for the 
code, but I think that shouldn't be much of a problem.

>> Annotating those function with __fpu or even saying to gcc that all code of
>> those files should go into a special text.fpu segment shouldn't be much of a
>> problem.
> Guess what the __fpu attribute does ;-)

Good to know that my suspicion how this is implemented was correct :)

> With the below patch (which is on to of newer versions of the objtool
> patches send earlier, let me know if you want a full set

Getting a branch somewhere would be perfect.

> ) that only
> converts a few files, but fully converts:
>
>    drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c
>
> But building it (and this is an absolute pain; when you're reworking
> this, can you pretty please also fix the Makefiles?), we get:
>
>    drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o: warning: objtool: dcn_validate_bandwidth()+0x34fa: FPU instruction outside of kernel_fpu_{begin,end}()
>
> $ ./scripts/faddr2line defconfig-build/drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o dcn_validate_bandwidth+0x34fa
> dcn_validate_bandwidth+0x34fa/0x57ce:
> dcn_validate_bandwidth at /usr/src/linux-2.6/defconfig-build/../drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.c:1293 (discriminator 5)
>
> # ./objdump-func.sh defconfig-build/drivers/gpu/drm/amd/amdgpu/../display/dc/calcs/dcn_calcs.o dcn_validate_bandwidth | grep 34fa
> 34fa     50fa:  f2 0f 10 b5 60 ff ff    movsd  -0xa0(%rbp),%xmm6
>
> Which seems to indicate there's still problms with the current code.

Making an educated guess I would say the compiler has no idea that it 
shouldn't use instructions which touch fp registers outside of 
kernel_fpu_{begin,end}().

Going to talk with the display team about this whole topic internally 
once more. Since this discussion already raised attention in our 
technical management it shouldn't be to much of a problem to get 
manpower to get this fixed properly.

Can we put this new automated check will be behind a configuration flag 
initially? Or at least make it a warning and not a hard error.

Thanks,
Christian.