Re: [PATCH v3 1/2] kernel/sys: add PR_GET_TASK_SIZE option to prctl(2)
On Fri, May 03, 2019 at 02:10:20PM -0400, Joel Savitz wrote: > +/* Get the process virtual memory size (i.e. the highest usable VM address) > */ > +#define PR_GET_TASK_SIZE 55 TASK_SIZE is in fact the lowest _un_usable address. :^)
Re: [PATCH v3 1/2] kernel/sys: add PR_GET_TASK_SIZE option to prctl(2)
On Fri, May 3, 2019 at 2:12 PM Joel Savitz wrote:
> 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.
A commit message shouldn't just describe what you're doing, but also
why you're doing it. Is this intended for processes that are running
on X86-64 and want to determine whether the system supports 5-level
paging, or something like that?
> +static int prctl_get_tasksize(void __user *uaddr)
> +{
> + unsigned long current_task_size, current_word_size;
> +
> + current_task_size = TASK_SIZE;
> + current_word_size = sizeof(unsigned long);
> +
> +#ifdef CONFIG_64BIT
> + /* On 64-bit architecture, we must check whether the current thread
> +* is running in 32-bit compat mode. If it is, we can simply cut
> +* the size in half. This avoids corruption of the userspace stack.
> +*/
> + if (test_thread_flag(TIF_ADDR32))
> + current_word_size >>= 1;
> +#endif
> +
> + return copy_to_user(uaddr, _task_size, current_word_size) ?
> -EFAULT : 0;
> +}
This function looks completely wrong; in particular, you're assuming
that the architecture is little-endian.
Make the value a u64, and you won't have these problems:
static int prctl_get_tasksize(u64 __user *uaddr)
{
return put_user(TASK_SIZE, uaddr) ? -EFAULT : 0;
}
A bunch of other new pieces of userspace API already use "u64" to
store userspace pointers and lengths to avoid compat issues.
> @@ -2486,6 +2506,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
> arg2, unsigned long, arg3,
> return -EINVAL;
> error = PAC_RESET_KEYS(me, arg2);
> break;
> + case PR_GET_TASK_SIZE:
> + error = prctl_get_tasksize((void *)arg2);
s/void */void __user */
Re: [PATCH v3 1/2] kernel/sys: add PR_GET_TASK_SIZE option to prctl(2)
On Fri, May 03, 2019 at 02:08:31PM -0700, Yury Norov wrote:
> On Fri, May 03, 2019 at 02:10:20PM -0400, Joel Savitz wrote:
> > 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.
> >
> > It is important that we account for the case of the userspace task
> > running in 32-bit compat mode on a 64-bit kernel. As such, we must be
> > careful to copy the correct number of bytes to userspace to avoid stack
> > corruption.
> >
> > Suggested-by: Yuri Norov
>
> I actually didn't suggest that. If you _really_ need TASK_SIZE to
> be exposed, I would suggest to expose it in kernel headers. TASK_SIZE
> is a compile-time information, and it may available for userspace at
> compile time as well.
>
> > Suggested-by: Alexey Dobriyan
> > Signed-off-by: Joel Savitz
> > ---
> > include/uapi/linux/prctl.h | 3 +++
> > kernel/sys.c | 23 +++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> > index 094bb03b9cc2..2c261c461952 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 (i.e. the highest usable VM
> > address) */
> > +#define PR_GET_TASK_SIZE 55
> > +
> > #endif /* _LINUX_PRCTL_H */
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 12df0e5434b8..709584400070 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2252,6 +2252,26 @@ static int propagate_has_child_subreaper(struct
> > task_struct *p, void *data)
> > return 1;
> > }
> >
> > +static int prctl_get_tasksize(void __user *uaddr)
> > +{
> > + unsigned long current_task_size, current_word_size;
> > +
> > + current_task_size = TASK_SIZE;
> > + current_word_size = sizeof(unsigned long);
> > +
> > +#ifdef CONFIG_64BIT
> > + /* On 64-bit architecture, we must check whether the current thread
> > +* is running in 32-bit compat mode. If it is, we can simply cut
> > +* the size in half. This avoids corruption of the userspace stack.
> > +*/
> > + if (test_thread_flag(TIF_ADDR32))
>
> It breaks build for all architectures except x86 since TIF_ADDR32 is
> defined for x86 only.
Or we could get TIF_32BIT also defined for x86 (same value of
TIF_ADDR32) and check for it instead. i.e.
...
#if defined(CONFIG_64BIT) && defined(TIF_32BIT)
if (test_thread_flag(TIF_32BIT))
...
which is also uglier and keeps adding unecessary complexity to a very
simple task. At this point, I think we just should give up on trying
this via prctl(2) and do it via /proc//status instead.
>
> In comment to v2 I suggested you to stick to fixed-size data type to
> avoid exactly this problem.
>
> NACK
>
> Yury
>
> > + current_word_size >>= 1;
> > +#endif
> > +
> > + return copy_to_user(uaddr, _task_size, current_word_size) ?
> > -EFAULT : 0;
> > +}
> > +
> > int __weak arch_prctl_spec_ctrl_get(struct task_struct *t, unsigned long
> > which)
> > {
> > return -EINVAL;
> > @@ -2486,6 +2506,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
> > arg2, unsigned long, arg3,
> > return -EINVAL;
> > error = PAC_RESET_KEYS(me, arg2);
> > break;
> > + case PR_GET_TASK_SIZE:
> > + error = prctl_get_tasksize((void *)arg2);
> > + break;
> > default:
> > error = -EINVAL;
> > break;
> > --
> > 2.18.1
Re: [PATCH v3 1/2] kernel/sys: add PR_GET_TASK_SIZE option to prctl(2)
On Fri, May 03, 2019 at 02:08:31PM -0700, Yury Norov wrote:
> On Fri, May 03, 2019 at 02:10:20PM -0400, Joel Savitz wrote:
> > 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.
> >
> > It is important that we account for the case of the userspace task
> > running in 32-bit compat mode on a 64-bit kernel. As such, we must be
> > careful to copy the correct number of bytes to userspace to avoid stack
> > corruption.
> >
> > Suggested-by: Yuri Norov
>
> I actually didn't suggest that. If you _really_ need TASK_SIZE to
> be exposed, I would suggest to expose it in kernel headers. TASK_SIZE
> is a compile-time information, and it may available for userspace at
> compile time as well.
>
TASK_SIZE is a runtime resolved macro, dependent on the thread currently
running on the CPU. It's not a compile time constant.
Anyways, it's proven that going prctl(2), although interesting, as
suggested by Alexey, wasn't worth the hassle as it poses more issues
than it can possibly solve.
A better way to get this value exposed to userspace is really through
/proc//status, where one can utilize TASK_SIZE_OF(mm->owner), or
simply mm->task_size, which seems to be properly assigned for each arch
> > Suggested-by: Alexey Dobriyan
> > Signed-off-by: Joel Savitz
> > ---
> > include/uapi/linux/prctl.h | 3 +++
> > kernel/sys.c | 23 +++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> > index 094bb03b9cc2..2c261c461952 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 (i.e. the highest usable VM
> > address) */
> > +#define PR_GET_TASK_SIZE 55
> > +
> > #endif /* _LINUX_PRCTL_H */
> > diff --git a/kernel/sys.c b/kernel/sys.c
> > index 12df0e5434b8..709584400070 100644
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -2252,6 +2252,26 @@ static int propagate_has_child_subreaper(struct
> > task_struct *p, void *data)
> > return 1;
> > }
> >
> > +static int prctl_get_tasksize(void __user *uaddr)
> > +{
> > + unsigned long current_task_size, current_word_size;
> > +
> > + current_task_size = TASK_SIZE;
> > + current_word_size = sizeof(unsigned long);
> > +
> > +#ifdef CONFIG_64BIT
> > + /* On 64-bit architecture, we must check whether the current thread
> > +* is running in 32-bit compat mode. If it is, we can simply cut
> > +* the size in half. This avoids corruption of the userspace stack.
> > +*/
> > + if (test_thread_flag(TIF_ADDR32))
>
> It breaks build for all architectures except x86 since TIF_ADDR32 is
> defined for x86 only.
>
> In comment to v2 I suggested you to stick to fixed-size data type to
> avoid exactly this problem.
>
> NACK
>
> Yury
>
> > + current_word_size >>= 1;
> > +#endif
> > +
> > + return copy_to_user(uaddr, _task_size, current_word_size) ?
> > -EFAULT : 0;
> > +}
> > +
> > int __weak arch_prctl_spec_ctrl_get(struct task_struct *t, unsigned long
> > which)
> > {
> > return -EINVAL;
> > @@ -2486,6 +2506,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
> > arg2, unsigned long, arg3,
> > return -EINVAL;
> > error = PAC_RESET_KEYS(me, arg2);
> > break;
> > + case PR_GET_TASK_SIZE:
> > + error = prctl_get_tasksize((void *)arg2);
> > + break;
> > default:
> > error = -EINVAL;
> > break;
> > --
> > 2.18.1
Re: [PATCH v3 1/2] kernel/sys: add PR_GET_TASK_SIZE option to prctl(2)
On Fri, May 03, 2019 at 02:10:20PM -0400, Joel Savitz wrote:
> 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.
>
> It is important that we account for the case of the userspace task
> running in 32-bit compat mode on a 64-bit kernel. As such, we must be
> careful to copy the correct number of bytes to userspace to avoid stack
> corruption.
>
> Suggested-by: Yuri Norov
I actually didn't suggest that. If you _really_ need TASK_SIZE to
be exposed, I would suggest to expose it in kernel headers. TASK_SIZE
is a compile-time information, and it may available for userspace at
compile time as well.
> Suggested-by: Alexey Dobriyan
> Signed-off-by: Joel Savitz
> ---
> include/uapi/linux/prctl.h | 3 +++
> kernel/sys.c | 23 +++
> 2 files changed, 26 insertions(+)
>
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index 094bb03b9cc2..2c261c461952 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 (i.e. the highest usable VM address)
> */
> +#define PR_GET_TASK_SIZE 55
> +
> #endif /* _LINUX_PRCTL_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 12df0e5434b8..709584400070 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2252,6 +2252,26 @@ static int propagate_has_child_subreaper(struct
> task_struct *p, void *data)
> return 1;
> }
>
> +static int prctl_get_tasksize(void __user *uaddr)
> +{
> + unsigned long current_task_size, current_word_size;
> +
> + current_task_size = TASK_SIZE;
> + current_word_size = sizeof(unsigned long);
> +
> +#ifdef CONFIG_64BIT
> + /* On 64-bit architecture, we must check whether the current thread
> + * is running in 32-bit compat mode. If it is, we can simply cut
> + * the size in half. This avoids corruption of the userspace stack.
> + */
> + if (test_thread_flag(TIF_ADDR32))
It breaks build for all architectures except x86 since TIF_ADDR32 is
defined for x86 only.
In comment to v2 I suggested you to stick to fixed-size data type to
avoid exactly this problem.
NACK
Yury
> + current_word_size >>= 1;
> +#endif
> +
> + return copy_to_user(uaddr, _task_size, current_word_size) ?
> -EFAULT : 0;
> +}
> +
> int __weak arch_prctl_spec_ctrl_get(struct task_struct *t, unsigned long
> which)
> {
> return -EINVAL;
> @@ -2486,6 +2506,9 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long,
> arg2, unsigned long, arg3,
> return -EINVAL;
> error = PAC_RESET_KEYS(me, arg2);
> break;
> + case PR_GET_TASK_SIZE:
> + error = prctl_get_tasksize((void *)arg2);
> + break;
> default:
> error = -EINVAL;
> break;
> --
> 2.18.1
