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
Content-Transfer-Encoding: quoted-printable
From:   Andy Lutomirski <luto@amacapital.net>
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH v2] x86/arch_prctl: Add ARCH_SET_XCR0 to set XCR0 per-thread
Date:   Tue, 7 Apr 2020 14:42:44 -0700
Message-Id: <32206246-48F6-4C03-B2C7-FC766B1254EB@amacapital.net>
References: <9921cb2e-a7cb-c1d0-b120-c08f06be7c7f@intel.com>
Cc:     Keno Fischer <keno@juliacomputing.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@kernel.org>,
        "H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Andi Kleen <andi@firstfloor.org>,
        Kyle Huey <khuey@kylehuey.com>,
        Robert O'Callahan <robert@ocallahan.org>
In-Reply-To: <9921cb2e-a7cb-c1d0-b120-c08f06be7c7f@intel.com>
To:     Dave Hansen <dave.hansen@intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Apr 7, 2020, at 1:21 PM, Dave Hansen <dave.hansen@intel.com> wrote:
>=20
> =EF=BB=BFOn 4/7/20 10:55 AM, Keno Fischer wrote:
>>> On Tue, Apr 7, 2020 at 12:27 PM Dave Hansen <dave.hansen@intel.com> wrot=
e:
>>>=20
>>>>> How does this work with things like xstateregs_[gs]et() where the form=
at
>>>>> of the kernel buffer and thus the kernel XCR0 is exposed as part of ou=
r
>>>>> ABI?  With this patch, wouldn't a debugger app see a state buffer that=

>>>>> looks invalid?
>>>>=20
>>>> Since those operate on the in-kernel buffer of these, which
>>>> in this patch always uses the unmodified XCR0, ptracers
>>>> should not observe a difference.
>>>=20
>>> Those operate on *BOTH* kernel and userspace buffers.  They copy between=

>>> them.  That's kinda the point. :)
>>=20
>> Right, what I meant was that in this patch the kernel level
>> xsaves that populates the struct fpu always runs with
>> an unmodified XCR0, so the contents of the xsave area
>> in struct fpu does not change layout (this is the major
>> change in this patch over v1).
>=20
> The userspace buffer is... a userspace buffer.  It is not and should not
> be tied to the format of the kernel buffer.
>=20
>> Are you referring to a ptracer which runs with a modified XCR0, and
>> assumes that the value it gets back from ptrace will have an
>> XSTATE_BV equal to its own observed XCR0 and thus get confused about
>> the layout of the buffer (or potentially have not copied all of the
>> relevant xstate because it calculated a wrong buffer size)?
>=20
> I don't think it's insane for a process to assume that it can XRSTOR a
> buffer that it gets back from ptrace.  That seems like something that
> could clearly be an ABI that apps depend on.
>=20
> Also, let's look at the comment about where XCR0 shows up in the ABI
> (arch/x86/include/asm/user.h):
>=20
>> * For now, only the first 8 bytes of the software usable bytes[464..471] w=
ill
>> * be used and will be set to OS enabled xstate mask (which is same as the=

>> * 64bit mask returned by the xgetbv's xCR0).
>=20
> That also makes it sound like we expect there to be a *SINGLE* value
> across the entire system.  It also makes me wonder *which* xgetbv is
> expected to match USER_XSTATE_XCR0_WORD.  It can't be the ptracee since
> we expect them to change XCR0.  It can't be the ptracer because they can
> use this new prctl too.  So does it refer to the kernel?  Or, should the
> new prctl() *disable* future ptrace()s?
>=20
>> If so, I think that's just a buggy ptracer. The kernel's xfeature
>> mask is available via ptrace and a well-behaved ptracer should use
>> that (e.g. gdb does, though it looks like it then also assumes that
>> the xstate has no holes, so it potentially gets the layout wrong
>> anyway).
>=20
> I'm trying to figure out what the semantics are of this whole thing.  It
> can't be "don't let userspace observe the real XCR0" because ptrace
> exposes that.  Is it, "make memory images portable, unless it's a memory
> image from ptrace"?
>=20
>> In general, I don't really want the modified XCR0 to affect
>> anything other than the particular instructions that depend
>> on it and maybe the signal frame (though as I said before,
>> I'm open to either here).
>=20
> Just remember that, in the end, we don't get to say what a good ptracer
> or bad ptracer is.  If they're expecting semantics that we've kept
> constant for 10 years, we change the semantics, and the app breaks, the
> kernel is in the wrong.
>=20
> I also don't feel like I have a good handle on what ptracers *do* with
> their XSAVE buffers that they get/set.  How many apps in a distro do
> something with this interface?

Most of them treat it as bytes to be blindly stuck back into the tracee.  So=
me of them will display some registers for the user=E2=80=99s benefit.  Mayb=
e a couple that I don=E2=80=99t know about do something odd.=