linux-kernel - Re: [patch V3 59/66] x86/fpu/signal: Move initial checks into fpu__sig

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, 22 Jun 2021 20:43:16 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Borislav Petkov <bp@...e.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Yu-cheng Yu <yu-cheng.yu@...el.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [patch V3 59/66] x86/fpu/signal: Move initial checks into fpu__sig_restore()

On Tue, Jun 22 2021 at 20:38, Thomas Gleixner wrote:

> On Tue, Jun 22 2021 at 19:35, Borislav Petkov wrote:
>> On Fri, Jun 18, 2021 at 04:19:22PM +0200, Thomas Gleixner wrote:
>>>  
>>> +	if (unlikely(!buf)) {
>>> +		fpu__clear_user_states(&current->thread.fpu);
>>
>> You could declare
>>
>> 	struct fpu *fpu = &tsk->thread.fpu;
>>
>> above so that it is easier to read, as this call is done twice.
>>
>> Also, you can do:
>>
>> 	int ret = 0;
>>
>> 	if (unlikely(!buf))
>> 		goto out;
>>
>> so that the exit paths converge at the end.
>
> I pondered, but look at the condition there. It gets unreadable.
>
> So I kept is as is because this is an intentional clear which returns
> success and the other is on error.
>
>>> +	if (!IS_ENABLED(CONFIG_X86_64) && !static_cpu_has(X86_FEATURE_FPU)) {
>>
>> cpu_feature_enabled()
>>
>>> +		return fpregs_soft_set(current, NULL, 0,
>>> +				       sizeof(struct user_i387_ia32_struct),
>>> +				       NULL, buf);
>>
>> Err, don't you need to catch retval into ret here and goto out, like
>> before, so that you can call fpu__clear_user_states() on error?
>
> Yes. Actually we should do that as a separate patch way earlier in the
> series. Sigh.

Bah, no. I screwed that up. Blush