[Devel] Re: [PATCH] c/r: [signal 2/3] checkpoint/restart of rlimit

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl at librato.com):
>> 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.

Oren.
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers