On Wed, Aug 07, 2019 at 05:48:29PM +0200, Oleg Nesterov wrote:
> On 08/06, Adrian Reber wrote:
> >
> > @@ -2530,12 +2530,14 @@ noinline static int
> > copy_clone_args_from_user(struct kernel_clone_args *kargs,
> > struct clone_args __user *uargs,
> > size_t size)
> > {
> > + struct pid_namespace *pid_ns = task_active_pid_ns(current);
> > struct clone_args args;
> >
> > if (unlikely(size > PAGE_SIZE))
> > return -E2BIG;
> >
> > - if (unlikely(size < sizeof(struct clone_args)))
> > + /* The struct needs to be at least the size of the original struct. */
> > + if (size < (sizeof(struct clone_args) - sizeof(__aligned_u64)))
> > return -EINVAL;
>
> slightly off-topic, but with or without this patch I do not understand
> -EINVAL. Can't we replace this check with
>
> if (size < sizeof(struct clone_args))
> memset((void*)&args + size, sizeof(struct clone_args) - size,
> 0);
>
> ?
>
> this way we can new members at the end of clone_args and this matches
> the "if (size > sizeof(struct clone_args))" block below which promises
> that whatever we add into clone_args a zero value should work.
Hm, I actually think we should define:
#define CLONE3_ARGS_SIZE_V0 64
#define CLONE3_ARGS_SIZE_V1 ...
and then later on for future extensions
#define CLONE3_ARGS_SIZE_V2 ...
then do
if (size < CLONE3_ARGS_SIZE_V0)
return -EINVAL;
then do what you suggested:
if (size < sizeof(struct clone_args))
memset((void*)&args + size, sizeof(struct clone_args) - size, 0);
>
>
> And if we do this
>
> > + if (size == sizeof(struct clone_args)) {
> > + /* Only check permissions if set_tid is actually set. */
> > + if (args.set_tid &&
> > + !ns_capable(pid_ns->user_ns, CAP_SYS_ADMIN))
> > + return -EPERM;
> > + kargs->set_tid = args.set_tid;
> > + }
>
> we can move this check into clone3_args_valid() or even copy_process()
>
> if (kargs->set_tid) {
> if (!ns_capable(...))
> return -EPERM;
> }
>
>
> Either way,
>
> > @@ -2585,6 +2595,10 @@ static bool clone3_args_valid(const struct
> > kernel_clone_args *kargs)
> > if (kargs->flags & ~CLONE_LEGACY_FLAGS)
> > return false;
> >
> > + /* Fail if set_tid is invalid */
> > + if (kargs->set_tid < 0)
> > + return false;
>
> I think it would be more clean to do this along with ns_capable() check,
> or along with the "set_tid >= pid_max" check in alloc_pid().
>
> I won't insist, but I do not really like the fact we check set_tid 3 times
> in copy_clone_args_from_user(), clone3_args_valid(), and alloc_pid().
Agreed on that part.
