Quoting Oren Laadan ([email protected]): > > > Serge E. Hallyn wrote: > > Quoting Oren Laadan ([email protected]): > >> This patch adds checkpoint and restart of rlimit information > >> that is part of shared signal_struct. > > > > ... > > > >> static int restore_signal(struct ckpt_ctx *ctx) > >> { > >> struct ckpt_hdr_signal *h; > >> + struct rlimit rlim; > >> + int i, ret; > >> > >> h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_SIGNAL); > >> if (IS_ERR(h)) > >> return PTR_ERR(h); > >> > >> - /* fill in later */ > >> - > >> + /* rlimit */ > >> + for (i = 0; i < RLIM_NLIMITS; i++) { > >> + rlim.rlim_cur = h->rlim[i].rlim_cur; > >> + rlim.rlim_max = h->rlim[i].rlim_max; > >> + ret = do_setrlimit(i, &rlim); > > > > ... > >> +int do_setrlimit(unsigned int resource, struct rlimit *new_rlim) > >> { > >> - struct rlimit new_rlim, *old_rlim; > >> + struct rlimit *old_rlim; > >> int retval; > >> > >> - if (resource >= RLIM_NLIMITS) > >> - return -EINVAL; > >> - if (copy_from_user(&new_rlim, rlim, sizeof(*rlim))) > >> - return -EFAULT; > >> - if (new_rlim.rlim_cur > new_rlim.rlim_max) > >> - return -EINVAL; > >> old_rlim = current->signal->rlim + resource; > >> - if ((new_rlim.rlim_max > old_rlim->rlim_max) && > >> + if ((new_rlim->rlim_max > old_rlim->rlim_max) && > >> !capable(CAP_SYS_RESOURCE)) > >> return -EPERM; > >> - if (resource == RLIMIT_NOFILE && new_rlim.rlim_max > sysctl_nr_open) > >> + if (resource == RLIMIT_NOFILE && new_rlim->rlim_max > sysctl_nr_open) > >> return -EPERM; > >> > >> - retval = security_task_setrlimit(resource, &new_rlim); > >> + retval = security_task_setrlimit(resource, new_rlim); > >> if (retval) > >> return retval; > >> > >> - if (resource == RLIMIT_CPU && new_rlim.rlim_cur == 0) { > >> + if (resource == RLIMIT_CPU && new_rlim->rlim_cur == 0) { > >> /* > >> * The caller is asking for an immediate RLIMIT_CPU > >> * expiry. But we use the zero value to mean "it was > >> * never set". So let's cheat and make it one second > >> * instead > >> */ > >> - new_rlim.rlim_cur = 1; > >> + new_rlim->rlim_cur = 1; > >> } > >> > >> task_lock(current->group_leader); > >> - *old_rlim = new_rlim; > >> + *old_rlim = *new_rlim; > >> task_unlock(current->group_leader); > >> > >> if (resource != RLIMIT_CPU) > >> @@ -1189,14 +1183,27 @@ SYSCALL_DEFINE2(setrlimit, unsigned int, resource, > >> struct rlimit __user *, rlim) > >> * very long-standing error, and fixing it now risks breakage of > >> * applications, so we live with it > >> */ > >> - if (new_rlim.rlim_cur == RLIM_INFINITY) > >> + if (new_rlim->rlim_cur == RLIM_INFINITY) > >> goto out; > >> > >> - update_rlimit_cpu(new_rlim.rlim_cur); > >> + update_rlimit_cpu(new_rlim->rlim_cur); > >> out: > >> return 0; > >> } > >> > >> +SYSCALL_DEFINE2(setrlimit, unsigned int, resource, struct rlimit __user > >> *, rlim) > >> +{ > >> + struct rlimit new_rlim; > >> + > >> + if (resource >= RLIM_NLIMITS) > >> + return -EINVAL; > >> + if (copy_from_user(&new_rlim, rlim, sizeof(*rlim))) > >> + return -EFAULT; > >> + if (new_rlim.rlim_cur > new_rlim.rlim_max) > >> + return -EINVAL; > > > > Should the above check go into do_setrlimit()? No sense trusting > > the data sent to sys_checkpoint() any more than the data sent to > > sys_setrlimit(). > > You are very correct. > > I wonder though: moving the first check will change the order of > input sanitizing, which will change the syscall behavior on bad > input. E.g, setrlimit(4096, NULL) used to return EINVAL but now > will return EFAULT. > > Not that I really care that much, but I've seen a similar case > that confused LTP scripts into seeing the "wrong" error from a > syscall and failing a test.
Heh, I could be wrong, but when you mess up 2 ways, I don't think the kernel needs to guarantee which one you'll be warned about :) Of course there are cases where that is well-defined (i.e. DAC-before-MAC). Maybe we should ask at linux-api? Putting the same check before both callers of do_setrlimit() isn't *that* bad, and I suppose we can put a comment above do_setrlimit() saying that that any new callers need to do that check themselves... -serge _______________________________________________ Containers mailing list [email protected] https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Devel mailing list [email protected] https://openvz.org/mailman/listinfo/devel
