Re: [PATCH v3 6/7] x86/tls,ptrace: provide regset access to the GDT

[Andy Lutomirski]

On Thu, Jun 21, 2018 at 2:18 PM H. Peter Anvin, Intel
<h.peter.anvin@xxxxxxxxx> wrote:
>
> From: "H. Peter Anvin" <hpa@xxxxxxxxxxxxxxx>
>
> 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