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  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:   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/ 

Powered by blists - more mailing lists