LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Alexander Potapenko <glider@google.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>,
	sunhaoyl@outlook.com, "the arch/x86 maintainers" <x86@kernel.org>
Subject: Re: [PATCH] fs/binfmt_elf.c: allocate initialized memory in fill_thread_core_info()
Date: Wed, 27 May 2020 14:08:34 +0200
Message-ID: <CAG_fn=Xvhdjhd97ChesvXScGhCC-kVp0yxT-W5L9WzaF2_4UjA@mail.gmail.com> (raw)
In-Reply-To: <20200526223817.GA3819674@ZenIV.linux.org.uk>

On Wed, May 27, 2020 at 12:38 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, May 25, 2020 at 12:45:35AM +0100, Al Viro wrote:
> > On Wed, May 13, 2020 at 04:33:49AM +0100, Al Viro wrote:
> >
> > > FWIW, what I'm going to do is
> > >     * make all callers of copy_regset_to_user() pass 0 as pos
> > > (there are very few exceptions - one on arm64, three on sparc32
> > > and five on sparc64; I hadn't dealt with arm64 one yet, but all
> > > cases on sparc are handled)
> >
> > [snip]
> >
> > Any of that would be easy to backport, though.  Several questions
> > regaring XSAVE and friends:
> >
> > * do we ever run on XSAVE/XSAVES-capable hardware with XFEATURE_FP
> > turned off?
> >
> > * is it possible for x86 to have gaps between the state components
> > area as reported by CPUID 0x0d?  IOW, can area for feature 2
> > (XFEATURE_YMM) to start *not* at 0x200 and can area for N start
> > not right after the end of area for N-1 for some N > 2?
> >
> > I think I have an easy-to-backport solution, but I'm really confused
> > about XFEATURE_FP situation...
>
> Folks, could you test the following?
>
> copy_xstate_to_kernel(): don't leave parts of destination uninitialized
>
> copy the corresponding pieces of init_fpstate into the gaps instead.
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Tested-by: Alexander Potapenko <glider@google.com>


> ---
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 32b153d38748..6a54e83d5589 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -957,18 +957,31 @@ static inline bool xfeatures_mxcsr_quirk(u64 xfeatures)
>         return true;
>  }
>
> -/*
> - * This is similar to user_regset_copyout(), but will not add offset to
> - * the source data pointer or increment pos, count, kbuf, and ubuf.
> - */
> -static inline void
> -__copy_xstate_to_kernel(void *kbuf, const void *data,
> -                       unsigned int offset, unsigned int size, unsigned int size_total)
> +static void fill_gap(unsigned to, void **kbuf, unsigned *pos, unsigned *count)
>  {
> -       if (offset < size_total) {
> -               unsigned int copy = min(size, size_total - offset);
> +       if (*pos < to) {
> +               unsigned size = to - *pos;
> +
> +               if (size > *count)
> +                       size = *count;
> +               memcpy(*kbuf, (void *)&init_fpstate.xsave + *pos, size);
> +               *kbuf += size;
> +               *pos += size;
> +               *count -= size;
> +       }
> +}
>
> -               memcpy(kbuf + offset, data, copy);
> +static void copy_part(unsigned offset, unsigned size, void *from,
> +                       void **kbuf, unsigned *pos, unsigned *count)
> +{
> +       fill_gap(offset, kbuf, pos, count);
> +       if (size > *count)
> +               size = *count;
> +       if (size) {
> +               memcpy(*kbuf, from, size);
> +               *kbuf += size;
> +               *pos += size;
> +               *count -= size;
>         }
>  }
>
> @@ -981,8 +994,9 @@ __copy_xstate_to_kernel(void *kbuf, const void *data,
>   */
>  int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int offset_start, unsigned int size_total)
>  {
> -       unsigned int offset, size;
>         struct xstate_header header;
> +       const unsigned off_mxcsr = offsetof(struct fxregs_state, mxcsr);
> +       unsigned count = size_total;
>         int i;
>
>         /*
> @@ -998,46 +1012,42 @@ int copy_xstate_to_kernel(void *kbuf, struct xregs_state *xsave, unsigned int of
>         header.xfeatures = xsave->header.xfeatures;
>         header.xfeatures &= ~XFEATURE_MASK_SUPERVISOR;
>
> +       if (header.xfeatures & XFEATURE_MASK_FP)
> +               copy_part(0, off_mxcsr,
> +                         &xsave->i387, &kbuf, &offset_start, &count);
> +       if (header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM))
> +               copy_part(off_mxcsr, MXCSR_AND_FLAGS_SIZE,
> +                         &xsave->i387.mxcsr, &kbuf, &offset_start, &count);
> +       if (header.xfeatures & XFEATURE_MASK_FP)
> +               copy_part(offsetof(struct fxregs_state, st_space), 128,
> +                         &xsave->i387.st_space, &kbuf, &offset_start, &count);
> +       if (header.xfeatures & XFEATURE_MASK_SSE)
> +               copy_part(xstate_offsets[XFEATURE_MASK_SSE], 256,
> +                         &xsave->i387.xmm_space, &kbuf, &offset_start, &count);
> +       /*
> +        * Fill xsave->i387.sw_reserved value for ptrace frame:
> +        */
> +       copy_part(offsetof(struct fxregs_state, sw_reserved), 48,
> +                 xstate_fx_sw_bytes, &kbuf, &offset_start, &count);
>         /*
>          * Copy xregs_state->header:
>          */
> -       offset = offsetof(struct xregs_state, header);
> -       size = sizeof(header);
> -
> -       __copy_xstate_to_kernel(kbuf, &header, offset, size, size_total);
> +       copy_part(offsetof(struct xregs_state, header), sizeof(header),
> +                 &header, &kbuf, &offset_start, &count);
>
> -       for (i = 0; i < XFEATURE_MAX; i++) {
> +       for (i = FIRST_EXTENDED_XFEATURE; i < XFEATURE_MAX; i++) {
>                 /*
>                  * Copy only in-use xstates:
>                  */
>                 if ((header.xfeatures >> i) & 1) {
>                         void *src = __raw_xsave_addr(xsave, i);
>
> -                       offset = xstate_offsets[i];
> -                       size = xstate_sizes[i];
> -
> -                       /* The next component has to fit fully into the output buffer: */
> -                       if (offset + size > size_total)
> -                               break;
> -
> -                       __copy_xstate_to_kernel(kbuf, src, offset, size, size_total);
> +                       copy_part(xstate_offsets[i], xstate_sizes[i],
> +                                 src, &kbuf, &offset_start, &count);
>                 }
>
>         }
> -
> -       if (xfeatures_mxcsr_quirk(header.xfeatures)) {
> -               offset = offsetof(struct fxregs_state, mxcsr);
> -               size = MXCSR_AND_FLAGS_SIZE;
> -               __copy_xstate_to_kernel(kbuf, &xsave->i387.mxcsr, offset, size, size_total);
> -       }
> -
> -       /*
> -        * Fill xsave->i387.sw_reserved value for ptrace frame:
> -        */
> -       offset = offsetof(struct fxregs_state, sw_reserved);
> -       size = sizeof(xstate_fx_sw_bytes);
> -
> -       __copy_xstate_to_kernel(kbuf, xstate_fx_sw_bytes, offset, size, size_total);
> +       fill_gap(size_total, &kbuf, &offset_start, &count);
>
>         return 0;
>  }



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

  reply index

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-19 10:08 glider
2020-04-19 10:10 ` Alexander Potapenko
2020-04-20 22:33 ` Andrew Morton
2020-04-20 22:41   ` Kees Cook
2020-04-21  3:42     ` Al Viro
2020-04-21  8:14       ` Alexander Potapenko
2020-05-12  1:09         ` Al Viro
2020-05-12  3:44           ` Al Viro
2020-05-12  8:20             ` Alexander Potapenko
2020-05-13  3:33               ` Al Viro
2020-05-24 23:45                 ` Al Viro
2020-05-26 22:38                   ` Al Viro
2020-05-27 12:08                     ` Alexander Potapenko [this message]
2020-05-27 19:04                     ` Borislav Petkov
2020-05-27 19:53                       ` Al Viro
2020-05-27 20:09                         ` Borislav Petkov
2020-04-21 12:54       ` Alexander Potapenko
2020-04-21 15:09         ` Jann Horn
2020-04-21 16:04           ` Yu-cheng Yu
2020-04-21 16:16             ` Jann Horn
2020-04-21 16:26               ` Yu-cheng Yu
2020-04-21 20:20               ` Kees Cook
2020-04-21  8:06     ` Alexander Potapenko
2020-05-27 21:55     ` Kees Cook
2020-04-21  8:00   ` Alexander Potapenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAG_fn=Xvhdjhd97ChesvXScGhCC-kVp0yxT-W5L9WzaF2_4UjA@mail.gmail.com' \
    --to=glider@google.com \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sunhaoyl@outlook.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git