linux-kernel - Re: [PATCH v2 0/2] sys/prctl: expose TASK

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]

Message-ID: <CAJu-Uz4rnMCR5-zHj=Ub1xZD=mmw3z1-mEg+M=qWXV3yO7OJkg@mail.gmail.com>
Date:   Thu, 2 May 2019 14:34:49 -0700
From:   Yury Norov <norov.maillist@...il.com>
To:     Joel Savitz <jsavitz@...hat.com>
Cc:     Cyrill Gorcunov <gorcunov@...il.com>,
        Waiman Long <longman@...hat.com>, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Kristina Martsenko <kristina.martsenko@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Micah Morton <mortonm@...omium.org>,
        Yang Shi <yang.shi@...ux.alibaba.com>,
        Jann Horn <jannh@...gle.com>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Rafael Aquini <aquini@...hat.com>,
        Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [PATCH v2 0/2] sys/prctl: expose TASK_SIZE value to userspace

чт, 2 мая 2019 г. в 14:23, Joel Savitz <jsavitz@...hat.com>:
>
> Yes, this the change, thanks to the suggestion of Yury Norov.

Joel, could you please stop top-posting?

> I also now explicitly mention the expected userspace destination type
> in the manpage patch.
>
> Best,
> Joel Savitz
>
>
> On Thu, May 2, 2019 at 5:10 PM Cyrill Gorcunov <gorcunov@...il.com> wrote:
> >
> > On Thu, May 02, 2019 at 05:01:38PM -0400, Waiman Long wrote:
> > >
> > > What did you change in v2 versus v1?
> >
> > Seems unsigned long long has been changed to unsigned long.

Sorry guys, I replied to Joel, but accidentally dropped the folks.
Find discussion below.

чт, 2 мая 2019 г. в 13:50, Joel Savitz <jsavitz@...hat.com>:
>
> While I disagree that kernel memory is exposed, as the 8-byte
> (unsigned long long) value of task_size is initialized by the
> assignment of TASK_SIZE, I agree with your suggestion, as the current
> code may corrupt the userspace stack of the caller unless provided
> with the address of an unsigned long long, an unusual type to store a
> value of word size.
>
> As such, I have adopted your suggestion and added type information to
> my manpage patch. Expect the v2 to be posted shortly.
>
> Thank you for your review.
>
> Best,
> Joel Savitz
>
> On Thu, May 2, 2019 at 3:41 PM Yury Norov <norov.maillist@...il.com> wrote:
> >
> > чт, 2 мая 2019 г. в 12:15, Joel Savitz <jsavitz@...hat.com>:
> > >
> > > When PR_GET_TASK_SIZE is passed to prctl, the kernel will attempt to
> > > copy the value of TASK_SIZE to the userspace address in arg2.
> >
> > but you copy the value of task_size.
> >
> > > Suggested-by: Alexey Dobriyan <adobriyan@...il.com>
> > > Signed-off-by: Joel Savitz <jsavitz@...hat.com>
> > > ---
> > >  include/uapi/linux/prctl.h |  3 +++
> > >  kernel/sys.c               | 10 ++++++++++
> > >  2 files changed, 13 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> > > index 094bb03b9cc2..2335fe0a8db8 100644
> > > --- a/include/uapi/linux/prctl.h
> > > +++ b/include/uapi/linux/prctl.h
> > > @@ -229,4 +229,7 @@ struct prctl_mm_map {
> > >  # define PR_PAC_APDBKEY                        (1UL << 3)
> > >  # define PR_PAC_APGAKEY                        (1UL << 4)
> > >
> > > +/* Get the process virtual memory size */
> > > +#define PR_GET_TASK_SIZE               55
> > > +
> > >  #endif /* _LINUX_PRCTL_H */
> > > diff --git a/kernel/sys.c b/kernel/sys.c
> > > index 12df0e5434b8..7ced7dbd035d 100644
> > > --- a/kernel/sys.c
> > > +++ b/kernel/sys.c
> > > @@ -2252,6 +2252,13 @@ static int propagate_has_child_subreaper(struct task_struct *p, void *data)
> > >         return 1;
> > >  }
> > >
> > > +static int prctl_get_tasksize(void __user * uaddr)
> > > +{
> > > +       unsigned long long task_size = TASK_SIZE;
> > > +       return copy_to_user(uaddr, &task_size, sizeof(unsigned long long))
> > > +                       ? -EFAULT : 0;
> > > +}
> > > +
> >
> > task_size is unsigned long. On 32-bit systems you will end up exposing 4 bytes
> > of kernel memory. You should switch to sizeof(unsigned long).
> >
> > Your code is broken for compat arches. Take a look at the definition
> > of TASK_SIZE
> > for arm64, for example.
> >
> > Thanks,
> > Yury