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. 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(). Oleg.