linux-kernel - Re: [PATCH v10 01/28] x86/fpu/xstate: Fix the state copy function to the XSTATE buffer

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: <114DDE57-4509-4167-9E77-D6F74C793941@intel.com>
Date:   Sun, 3 Oct 2021 22:34:58 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     "bp@...e.de" <bp@...e.de>, "Lutomirski, Andy" <luto@...nel.org>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "lenb@...nel.org" <lenb@...nel.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Macieira, Thiago" <thiago.macieira@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 01/28] x86/fpu/xstate: Fix the state copy function to
 the XSTATE buffer

On Oct 1, 2021, at 05:44, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Wed, Aug 25 2021 at 08:53, Chang S. Bae wrote:
>> Harden copy_uabi_to_xstate() so that it can handle the case where
>> __raw_xsave() returns NULL.
> 
> That's an implementation detail, but does not explain why this can
> happen and what this patch is about.
> 
>> This does not happen in practice today, but theoretically could happen
>> in the future.
> 
> So what does the patch "fix"? When the subject says "Fix..." then I'm
> expecting a bug in the code to be fixed.
> 
> There is none because the use case which can trip over this does not
> exist today. You are adding it later.
> 
> Subject: .....: Prepare copy_uabi_to_xstate() to handle dynamic features
> 
> or something like that along with a reasonable explanation.
> 
> But in a later patch you add in the very same function:
> 
>> +    hdr.xfeatures &= fpu->state_mask;
> 
> which prevents that already because __raw_xsave_addr() is not invoked
> for the zeroed bits in hdr.xfeatures:
> 
>> 		if (hdr.xfeatures & mask) {
>> 			void *dst = __raw_xsave_addr(xsave, i);
> 
> Confused.
> 
> I'm not against the change per se, but I'm not accepting changelogs
> which make no sense at all. News at 11.

These two had been in the same patch that updates state copy functions for
dynamic features. It was suggested to move this to the patch, where
->state_mask is added:

    I don't know where this hunk belongs to...

    Maybe as a completely separate patch which fixes the case where
    __raw_xsave_addr() can in the very unlikely event return NULL...

I now consider here the NULL pointer check is not that needed in this series.
I’m going to drop this. But maybe I can do this separately later if needed.

Thanks,
Chang

[1] https://lore.kernel.org/lkml/YRz3EWQl7pHEahdF@zn.tnic/