On Thu, Jul 23, 2009 at 09:39:49PM -0400, Oren Laadan wrote:
>
>
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl@xxxxxxxxxxx):
> >> 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.
Unless the syscall documentation (if any) defines an order it checks
things I think such a test is broken. If multiple "things" are wrong with
a syscall then userspace should expect an error for any one of them.
Cheers,
-Matt
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
