linux-kernel - Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features

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: <04f821e6d4a8a736e6df2eb73ce811022cd42537.camel@intel.com>
Date:   Mon, 13 Mar 2023 16:10:14 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "bp@...en8.de" <bp@...en8.de>
CC:     "david@...hat.com" <david@...hat.com>,
        "bsingharora@...il.com" <bsingharora@...il.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "dethoma@...rosoft.com" <dethoma@...rosoft.com>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "pavel@....cz" <pavel@....cz>, "oleg@...hat.com" <oleg@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "jamorris@...ux.microsoft.com" <jamorris@...ux.microsoft.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "Schimpe, Christina" <christina.schimpe@...el.com>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "debug@...osinc.com" <debug@...osinc.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "john.allen@....com" <john.allen@....com>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>
Subject: Re: [PATCH v7 38/41] x86/fpu: Add helper for initing features

On Mon, 2023-03-13 at 12:03 +0100, Borislav Petkov wrote:
> On Mon, Mar 13, 2023 at 02:45:08AM +0000, Edgecombe, Rick P wrote:
> > These two are from the existing code. Basically they get extracted
> > into
> > a new function.
> 
> I know but you can fix them while at it.

Ok.

> 
> > I did it up, and it makes the caller code cleaner. But I'm not sure
> > what to think of it. Is this not mixing two operations together?
> > Today
> > get_xsave_addr() pretty much just gets a buffer offset with some
> > checks. Now it would compute the offset and also silently go off
> > and
> > changes the buffer.
> 
> Ok, so why don't you write the call site this way instead:
> 
>         cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
>         if (!cetregs) {
>                 if (xfeature_saved(xsave, XFEATURE_CET_USER)) {
>                         WARN("something's wrong with this buffer")
>                         return ...;
>                 }
> 
>                 /* Not saved, initialize it */
>                 init_xfeature(xsave, XFEATURE_CET_USER));
>         }
> 
>         cetregs = get_xsave_addr(xsave, XFEATURE_CET_USER);
>         if (!cetregs) {
>                 WARN_ON("WTF")
>                 return -ENODEV;
>         }
> 
> Now it is clear what happens and it is a common code pattern of
> trying
> to get something and initializing it if it wasn't initialized yet,
> and
> then retrying...
> 
> Hmm?

This seems more clear. I'm sorry for the noise here though, because
this has made me realize that the initing logic should never be hit. We
used to support the full CET_U state in ptrace, but then dropped it to
just the SSP and only allowed it when shadow stack is active. This
means that CET_U will always have at least the CET_SHSTK_EN bit set and
so not be in the init state. So this can probably just warn and bail if
it sees an init state.

Unless the extra logic seems more robust? But it is always nice when
the chance comes to drop a patch out of this thing...