Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl@xxxxxxxxxxx):
>>
>> 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.
>
> 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
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
