Serge E. Hallyn wrote: > 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?
I totally agree with you - I don't think it's an API issue. I only wonder whether this would cause an LTP test or a libc test to fail (because it expected one error and got another). Of course, it would be a false negative, but would still happen. Oh, well .. I'll just assume it doesn't break anything unless it is proved wrong :p Oren. > > 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
