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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Jun 2018 07:43:47 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     "H. Peter Anvin" <h.peter.anvin@...el.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "H. Peter Anvin" <hpa@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Lutomirski <luto@...nel.org>,
        "Bae, Chang Seok" <chang.seok.bae@...el.com>,
        "Metzger, Markus T" <markus.t.metzger@...el.com>
Subject: Re: [PATCH v3 6/7] x86/tls,ptrace: provide regset access to the GDT

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