Messages in this thread Patch in this message | | | Date | Wed, 7 Aug 2019 20:20:24 +0200 | From | Christian Brauner <> | Subject | Re: [PATCH v3 1/2] fork: extend clone3() to support CLONE_SET_TID |
| |
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.
| |