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;
MIME-Version: 1.0
References: <20180621211754.12757-1-h.peter.anvin@intel.com> <20180621211754.12757-7-h.peter.anvin@intel.com>
In-Reply-To: <20180621211754.12757-7-h.peter.anvin@intel.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Fri, 22 Jun 2018 07:43:47 -0700
Message-ID: <CALCETrV-cmjyFmuf-AdrdZU4J1Gq88cEC3JLKme3oxm9Jy_PMQ@mail.gmail.com>
Subject: Re: [PATCH v3 6/7] x86/tls,ptrace: provide regset access to the GDT
To:     "H. Peter Anvin" <h.peter.anvin@intel.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "H. Peter Anvin" <hpa@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andrew Lutomirski <luto@kernel.org>,
        "Bae, Chang Seok" <chang.seok.bae@intel.com>,
        "Metzger, Markus T" <markus.t.metzger@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jun 21, 2018 at 2:18 PM H. Peter Anvin, Intel
<h.peter.anvin@intel.com> wrote:
>
> From: "H. Peter Anvin" <hpa@linux.intel.com>
>
> Provide access to the user-visible part of the GDT via a regset in
> ptrace().  Note that we already provide a regset for the TLS area part
> of the GDT; these can trivially be unified by looking at the contents
> of the regset structure, especially since the TLS area is the only
> user-modifiable part of the GDT.

This seems reasonable, although I'm not sure I see the point of making
REGSET_GDT writable.  (I see the point of making the LDT writable, but
that's a different story.)

> +static int gdt_get(struct task_struct *target,
> +                  const struct user_regset *regset,
> +                  unsigned int pos, unsigned int count,
> +                  void *kbuf, void __user *ubuf)
> +{
> +       struct desc_struct gdt_copy[GDT_LAST_USER + 1];
> +       const struct desc_struct *p;
> +       struct user_desc udesc;
> +       unsigned int index, endindex;
> +       int err;
> +
> +       if (pos % sizeof(struct user_desc))
> +               return -EINVAL;
> +
> +       /* Get a snapshot of the GDT from an arbitrary CPU */
> +       memcpy(gdt_copy, get_current_gdt_ro(), sizeof(gdt_copy));
> +
> +       /* Copy over the TLS area */
> +       memcpy(&gdt_copy[GDT_ENTRY_TLS_MIN], target->thread.tls_array,
> +              sizeof(target->thread.tls_array));
> +
> +       /* Descriptor zero is never accessible */
> +       memset(&gdt_copy[0], 0, sizeof(gdt_copy[0]));

I think you should also mask out all system segments and all RPL != 3 segments.

> +static int gdt_active(struct task_struct  *target,
> +                     const struct user_regset *regset)
> +{
> +       (void)target;
> +       return GDT_LAST_USER + 1;
> +}

I can't find the code, if any, that calls ->active.  But, if it
exists, is the result exposed to user space at all?  If so, I think
this should return the maximum theoretical size of the GDT.

> +       [REGSET_GDT] = {
> +               .core_note_type = NT_X86_GDT,
> +               .n = LDT_ENTRIES, /* Theoretical maximum */
> +               .size = sizeof(struct user_desc),
> +               .align = sizeof(struct user_desc),
> +               .active = gdt_active,
> +               .get = gdt_get, .set = regset_gdt_set

As above, barring a reason why it's useful, I think that removing .set
would make sense.

--Andy