Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1556830342-32307-1-git-send-email-jsavitz@redhat.com>
 <8bb9fe29-65d3-e977-1932-4a2f17ead333@redhat.com> <20190502211002.GG2488@uranus.lan>
 <CAL1p7m6sYp_A4PB-K9gc8QKnPLHe92y2yMq7TqnUJuQPqR+MOA@mail.gmail.com>
In-Reply-To: <CAL1p7m6sYp_A4PB-K9gc8QKnPLHe92y2yMq7TqnUJuQPqR+MOA@mail.gmail.com>
From:   Yury Norov <norov.maillist@gmail.com>
Date:   Thu, 2 May 2019 14:34:49 -0700
Message-ID: <CAJu-Uz4rnMCR5-zHj=Ub1xZD=mmw3z1-mEg+M=qWXV3yO7OJkg@mail.gmail.com>
Subject: Re: [PATCH v2 0/2] sys/prctl: expose TASK_SIZE value to userspace
To:     Joel Savitz <jsavitz@redhat.com>
Cc:     Cyrill Gorcunov <gorcunov@gmail.com>,
        Waiman Long <longman@redhat.com>, linux-kernel@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
        Kristina Martsenko <kristina.martsenko@arm.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        YueHaibing <yuehaibing@huawei.com>,
        Micah Morton <mortonm@chromium.org>,
        Yang Shi <yang.shi@linux.alibaba.com>,
        Jann Horn <jannh@google.com>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Rafael Aquini <aquini@redhat.com>,
        Michael Kerrisk <mtk.manpages@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

=D1=87=D1=82, 2 =D0=BC=D0=B0=D1=8F 2019 =D0=B3. =D0=B2 14:23, Joel Savitz <=
jsavitz@redhat.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@gmail.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.

=D1=87=D1=82, 2 =D0=BC=D0=B0=D1=8F 2019 =D0=B3. =D0=B2 13:50, Joel Savitz <=
jsavitz@redhat.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@gmail.com> wrot=
e:
> >
> > =D1=87=D1=82, 2 =D0=BC=D0=B0=D1=8F 2019 =D0=B3. =D0=B2 12:15, Joel Savi=
tz <jsavitz@redhat.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@gmail.com>
> > > Signed-off-by: Joel Savitz <jsavitz@redhat.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(struc=
t task_struct *p, void *data)
> > >         return 1;
> > >  }
> > >
> > > +static int prctl_get_tasksize(void __user * uaddr)
> > > +{
> > > +       unsigned long long task_size =3D TASK_SIZE;
> > > +       return copy_to_user(uaddr, &task_size, sizeof(unsigned long l=
ong))
> > > +                       ? -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