Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RESEND PATCH v6 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <2B69DB9F-A3FC-4C60-BA51-E11EB9C5877D@intel.com>
Date:   Wed, 1 May 2019 17:29:55 -0700
Cc:     Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        "H . Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@linux.intel.com>,
        "Shankar, Ravi V" <ravi.v.shankar@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <F1D424D8-B210-47E3-9E56-FFFF0305BF86@amacapital.net>
References: <1552680405-5265-1-git-send-email-chang.seok.bae@intel.com> <1552680405-5265-9-git-send-email-chang.seok.bae@intel.com> <alpine.DEB.2.21.1903251003090.1798@nanos.tec.linutronix.de> <alpine.DEB.2.21.1904050007050.1802@nanos.tec.linutronix.de> <5DCF2089-98EC-42D3-96C3-6ECCDA0B18E2@amacapital.net> <C79FA889-BD9B-4427-902F-52EE33A3E6EF@intel.com> <CALCETrV4zACb9L_FaU12ZF1O6_vjVyGrcyWwk-mfSUhyxGMXJA@mail.gmail.com> <0816B012-44E8-40FB-8003-33C4841CD0E1@intel.com> <7029A32B-958E-4C1E-8B5F-D49BA68E4755@intel.com> <2863FA6C-F783-4322-9A01-4A2B8A7817A3@amacapital.net> <2B69DB9F-A3FC-4C60-BA51-E11EB9C5877D@intel.com>
To:     "Bae, Chang Seok" <chang.seok.bae@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On May 1, 2019, at 2:04 PM, Bae, Chang Seok <chang.seok.bae@intel.com> wro=
te:
>=20
>=20
>> On May 1, 2019, at 13:25, Andy Lutomirski <luto@amacapital.net> wrote:
>>=20
>>=20
>>=20
>>> On May 1, 2019, at 1:21 PM, Bae, Chang Seok <chang.seok.bae@intel.com> w=
rote:
>>>=20
>>>=20
>>>>> On May 1, 2019, at 11:01, Bae, Chang Seok <chang.seok.bae@intel.com> w=
rote:
>>>>>=20
>>>>> On May 1, 2019, at 10:40, Andy Lutomirski <luto@kernel.org> wrote:
>>>>>=20
>>>>>> On Wed, May 1, 2019 at 6:52 AM Bae, Chang Seok <chang.seok.bae@intel.=
com> wrote:
>>>>>>=20
>>>>>>=20
>>>>>>> On Apr 5, 2019, at 06:50, Andy Lutomirski <luto@amacapital.net> wrot=
e:
>>>>>>>=20
>>>>>>> Furthermore, if you folks even want me to review this series, the pt=
race tests need to be in place.  On inspection of the current code (after th=
e debacle a few releases back), it appears the SETREGSET=E2=80=99s effect de=
pends on the current values in the registers =E2=80=94 it does not actually s=
eem to reliably load the whole state. So my confidence will be greatly incre=
ased if your series first adds a test that detects that bug (and fails!), th=
en fixes the bug in a tiny little patch, then adds FSGSBASE, and keeps the t=
est working.
>>>>>>>=20
>>>>>>=20
>>>>>> I think I need to understand the issue. Appreciate if you can elabora=
te a little bit.
>>>>>>=20
>>>>>=20
>>>>> This patch series gives a particular behavior to PTRACE_SETREGS and
>>>>> PTRACE_POKEUSER.  There should be a test case that validates that
>>>>> behavior, including testing the weird cases where gs !=3D 0 and gsbase=

>>>>> contains unusual values.  Some existing tests might be pretty close to=

>>>>> doing what's needed.
>>>>>=20
>>>>> Beyond that, the current putreg() code does this:
>>>>>=20
>>>>> case offsetof(struct user_regs_struct,gs_base):
>>>>>    /*
>>>>>     * Exactly the same here as the %fs handling above.
>>>>>     */
>>>>>    if (value >=3D TASK_SIZE_MAX)
>>>>>        return -EIO;
>>>>>    if (child->thread.gsbase !=3D value)
>>>>>        return do_arch_prctl_64(child, ARCH_SET_GS, value);
>>>>>    return 0;
>>>>>=20
>>>>> and do_arch_prctl_64(), in turn, does this:
>>>>>=20
>>>>> case ARCH_SET_GS: {
>>>>>    if (unlikely(arg2 >=3D TASK_SIZE_MAX))
>>>>>        return -EPERM;
>>>>>=20
>>>>>    preempt_disable();
>>>>>    /*
>>>>>     * ARCH_SET_GS has always overwritten the index
>>>>>     * and the base. Zero is the most sensible value
>>>>>     * to put in the index, and is the only value that
>>>>>     * makes any sense if FSGSBASE is unavailable.
>>>>>     */
>>>>>    if (task =3D=3D current) {
>>>>>     [not used for ptrace]
>>>>>    } else {
>>>>>        task->thread.gsindex =3D 0;
>>>>>        x86_gsbase_write_task(task, arg2);
>>>>>    }
>>>>>=20
>>>>>    ...
>>>>>=20
>>>>> So writing the value that was already there to gsbase via putreg()
>>>>> does nothing, but writing a *different* value implicitly clears gs,
>>>>> but writing a different value will clear gs.
>>>>>=20
>>>>> This behavior is, AFAICT, complete nonsense.  It happens to work
>>>>> because usually gdb writes the same value back, and, in any case, gs
>>>>> comes *after* gsbase in user_regs_struct, so gs gets replaced anyway.
>>>>> But I think that this behavior should be fixed up and probably tested.=

>>>>> Certainly the behavior should *not* be the same on a fsgsbase kernel,
>>>>> and and the fsgsbase behavior definitely needs a selftest.
>>>>=20
>>>> Okay, got the point; now crystal clear.
>>>>=20
>>>> I have my own test case for that though, need to find a very simple and=

>>>> acceptable solution.
>>>>=20
>>>=20
>>> One solution that I recall, HPA once suggested, is:
>>>  Write registers in a reverse order from user_regs_struct, for SETREGS
>>>=20
>>> Assuming these for clarification, first:
>>>  * old and new index !=3D 0
>>>  * taking GS as an example though, should be the same with FS
>>>=20
>>> Then, interesting cases would be something like these, without FSGSBASE:=

>>>  Case (a), when index only changed to (new index):
>>>      (Then, the result after SETREGS would be)
>>>      GS =3D (new index), GSBASE =3D the base fetched from (new index)
>>>  Case (b), when base only changed to (new base):
>>>  Case (c), when both are changed:
>>>      GS =3D 0, GSBASE =3D (new base)
>>>=20
>>> Now, with FSGSBASE:
>>>  Case (a):
>>>      GS =3D (new index), GSBASE =3D (old base)
>>>  Case (b):
>>>      GS =3D (old index), GSBASE =3D (new base)
>>>  Case (c):
>>>      GS =3D (new index), GSBASE =3D (new base)
>>>=20
>>> As a reference, today's kernel behavior, without FSGSBASE:
>>>  Case (a):
>>>      GS =3D (new index), GSBASE =3D the base fetched from (new index)
>>>  Case (b):
>>>      GS =3D (old index), GSBASE =3D (old base)
>>>  Case (c):
>>>      GS =3D (new index), GSBASE =3D the base fetched from (new index)
>>>=20
>>> Now, with that reverse ordering and taking that "GSBASE is important" [1=
],
>>> it looks like to be working in terms of its base value:
>>>  Case (b) and (c) will behave the same as with FSGSBASE
>>>  Case (a) still differs between w/ and w/o FSGSBASE.
>>>      Well, I'd say this bit comes from the 'new model' vs. the 'leagcy
>>>      model'. So, then okay with that. Any thoughts?
>>>=20
>>>=20
>>>=20
>>=20
>> This seems more complicated than needed.  How about we just remove all th=
e magic and make putreg on the base registers never change the selector.
>>=20
>=20
> Hmm, just wonder what's benefit in terms of making a non-FSGSBASE system
> behave  more similar to one with FSGSBASE (although I would buy that remov=
al).

Simplicity. The current behavior is IMO nuts.

> Well, if we're okay with such divergence, maybe that's it.
>=20
>> As far as I can tell, the only downside is that, on a non-FSGSBASE kernel=
, setting only the base if the selector already has a nonzero value won=E2=80=
=99t work, but I would be quite surprised if this breaks anything.
>=20
>=20
>=20