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: <7029A32B-958E-4C1E-8B5F-D49BA68E4755@intel.com>
Date:   Wed, 1 May 2019 13:25:56 -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: <2863FA6C-F783-4322-9A01-4A2B8A7817A3@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>
To:     "Bae, Chang Seok" <chang.seok.bae@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On May 1, 2019, at 1:21 PM, Bae, Chang Seok <chang.seok.bae@intel.com> wro=
te:
>=20
>=20
>>> On May 1, 2019, at 11:01, Bae, Chang Seok <chang.seok.bae@intel.com> wro=
te:
>>>=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> wrote:=

>>>>>=20
>>>>> Furthermore, if you folks even want me to review this series, the ptra=
ce tests need to be in place.  On inspection of the current code (after the d=
ebacle a few releases back), it appears the SETREGSET=E2=80=99s effect depen=
ds on the current values in the registers =E2=80=94 it does not actually see=
m to reliably load the whole state. So my confidence will be greatly increas=
ed if your series first adds a test that detects that bug (and fails!), then=
 fixes the bug in a tiny little patch, then adds FSGSBASE, and keeps the tes=
t working.
>>>>>=20
>>>>=20
>>>> I think I need to understand the issue. Appreciate if you can elaborate=
 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

This seems more complicated than needed.  How about we just remove all the m=
agic and make putreg on the base registers never change the selector.

As far as I can tell, the only downside is that, on a non-FSGSBASE kernel, s=
etting only the base if the selector already has a nonzero value won=E2=80=99=
t work, but I would be quite surprised if this breaks anything.=