Re: [PATCH v2 0/2] sys/prctl: expose TASK_SIZE value to userspace

[Yury Norov]

ÑÑ, 2 ÐÐÑ 2019 Ð. Ð 14:23, Joel Savitz <jsavitz@xxxxxxxxxx>:
>
> 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@xxxxxxxxx> 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@xxxxxxxxxx>:
>
> 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@xxxxxxxxx> wrote:
> >
> > ÑÑ, 2 ÐÐÑ 2019 Ð. Ð 12:15, Joel Savitz <jsavitz@xxxxxxxxxx>:
> > >
> > > 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@xxxxxxxxx>
> > > Signed-off-by: Joel Savitz <jsavitz@xxxxxxxxxx>
> > > ---
> > >  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