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  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:   Thu, 19 Nov 2020 21:07:09 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...e.de>,
        Andrew Lutomirski <luto@...nel.org>, X86 ML <x86@...nel.org>,
        Len Brown <len.brown@...el.com>,
        Dave Hansen <dave.hansen@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 15/22] x86/fpu/xstate: Support ptracer-induced xstate
 area expansion

On Thu, Nov 19, 2020 at 3:37 PM Chang S. Bae <chang.seok.bae@...el.com> wrote:
>
> ptrace() may request an update to task->fpu that has not yet been
> allocated. Detect this case and allocate task->fpu to support the request.
> Also, disable the (now unnecessary) associated first-use fault.
>
> No functional change until the kernel supports dynamic user states.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Reviewed-by: Len Brown <len.brown@...el.com>
> Cc: x86@...nel.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  arch/x86/kernel/fpu/regset.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
> index 8d863240b9c6..6b9d0c0a266d 100644
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -125,6 +125,35 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
>
>         xsave = __xsave(fpu);
>
> +       /*
> +        * When a ptracer attempts to write any state in task->fpu but not allocated,
> +        * it dynamically expands the xstate area of fpu->state_ptr.
> +        */
> +       if (count > get_xstate_size(fpu->state_mask)) {
> +               unsigned int offset, size;
> +               struct xstate_header hdr;
> +               u64 mask;
> +
> +               offset = offsetof(struct xregs_state, header);
> +               size = sizeof(hdr);
> +
> +               /* Retrieve XSTATE_BV */
> +               if (kbuf) {
> +                       memcpy(&hdr, kbuf + offset, size);
> +               } else {
> +                       ret = __copy_from_user(&hdr, ubuf + offset, size);
> +                       if (ret)
> +                               return ret;
> +               }
> +
> +               mask = hdr.xfeatures & xfeatures_mask_user_dynamic;
> +               if (!mask) {
> +                       ret = alloc_xstate_area(fpu, mask, NULL);
> +                       if (ret)
> +                               return ret;
> +               }
> +       }
> +

This whole function is garbage.  The count parameter is entirely
ignored except that the beginning of the function compares it to the
constant known size.  Now that it's dynamic, you need to actually
validate the count.  Right now, you will happily overrun the buffer if
the mask in the buffer isn't consistent with count.

Please add some real bounds checking here.

Powered by blists - more mailing lists