Date:	Sun, 4 Oct 2009 12:50:45 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Marcin Slusarz <marcin.slusarz@...il.com>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [PATCH 1/3] extend get/setrlimit to support setting rlimits
	external to a process (v3)

On Sun, Oct 04, 2009 at 02:14:49PM +0200, Marcin Slusarz wrote:
> Neil Horman wrote:
> > Augment /proc/<pid>/limits file to support limit setting
> >(...)
> >  /* Display limits for a process */
> > -static int proc_pid_limits(struct task_struct *task, char *buffer)
> > +static ssize_t proc_pid_limit_read(struct file *file, char __user *buf,
> > +			size_t count, loff_t *ppos)
> >  {
> >  	unsigned int i;
> > -	int count = 0;
> >  	unsigned long flags;
> > -	char *bufptr = buffer;
> > +	char *bufptr;
> > +	size_t bcount = 0;
> > +	size_t ccount = 0;
> > +	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
> >  
> >  	struct rlimit rlim[RLIM_NLIMITS];
> >  
> > +	bufptr = kzalloc(PAGE_SIZE, GFP_KERNEL);
> > +	if (!bufptr)
> > +		goto out;
> > +
> >  	if (!lock_task_sighand(task, &flags))
> > -		return 0;
> > +		goto out;
> 
> memory leak (bufptr)
> 
Gah, thanks!

> >  	memcpy(rlim, task->signal->rlim, sizeof(struct rlimit) * RLIM_NLIMITS);
> >  	unlock_task_sighand(task, &flags);
> >  
> >  	/*
> >  	 * print the file header
> >  	 */
> > -	count += sprintf(&bufptr[count], "%-25s %-20s %-20s %-10s\n",
> > +	bcount += sprintf(&bufptr[bcount], "%-25s %-20s %-20s %-10s\n",
> >  			"Limit", "Soft Limit", "Hard Limit", "Units");
> >  
> >  	for (i = 0; i < RLIM_NLIMITS; i++) {
> >  		if (rlim[i].rlim_cur == RLIM_INFINITY)
> > -			count += sprintf(&bufptr[count], "%-25s %-20s ",
> > +			bcount += sprintf(&bufptr[bcount], "%-25s %-20s ",
> >  					 lnames[i].name, "unlimited");
> >  		else
> > -			count += sprintf(&bufptr[count], "%-25s %-20lu ",
> > +			bcount += sprintf(&bufptr[bcount], "%-25s %-20lu ",
> >  					 lnames[i].name, rlim[i].rlim_cur);
> >  
> >  		if (rlim[i].rlim_max == RLIM_INFINITY)
> > -			count += sprintf(&bufptr[count], "%-20s ", "unlimited");
> > +			bcount += sprintf(&bufptr[bcount], "%-20s ",
> > +					 "unlimited");
> >  		else
> > -			count += sprintf(&bufptr[count], "%-20lu ",
> > +			bcount += sprintf(&bufptr[bcount], "%-20lu ",
> >  					 rlim[i].rlim_max);
> >  
> >  		if (lnames[i].unit)
> > -			count += sprintf(&bufptr[count], "%-10s\n",
> > +			bcount += sprintf(&bufptr[bcount], "%-10s\n",
> >  					 lnames[i].unit);
> >  		else
> > -			count += sprintf(&bufptr[count], "\n");
> > +			bcount += sprintf(&bufptr[bcount], "\n");
> > +	}
> > +
> > +	if (*ppos >= bcount)
> > +		goto out_task;
> 
> again
> 
ditto.

> > +
> > +	ccount = min(count, (size_t)(bcount-(*ppos)));
> > +	ccount = ccount - copy_to_user(buf, &bufptr[*ppos], ccount);
> > +	*ppos += ccount;
> > +	kfree(bufptr);
> > +out_task:
> > +	put_task_struct(task);
> > +out:
> > +	return ccount;
> > +}
> > +
> > +static ssize_t proc_pid_limit_write(struct file *file, const char __user *buf,
> > +		size_t count, loff_t *ppos)
> > +{
> > +	char *buffer;
> > +	char *element, *vmc, *vmm;
> > +	struct rlimit new_rlim;
> > +	unsigned long flags;
> > +	int i;
> > +	int index = -1;
> > +	size_t wcount = 0;
> > +	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
> > +
> > +
> > +	if (*ppos != 0)
> > +		goto out;
> > +
> > +	if (count > 128)
> > +		goto out;
> > +	buffer = kzalloc(128, GFP_KERNEL);
> > +
> > +	if (!buffer)
> > +		goto out;
> 
> element, vmc, vmm are not initialized and you kfree them in this case
> 
Yep, I'll fix that

> > +
> > +	element = kzalloc(sizeof(buffer), GFP_KERNEL);
> > +	vmc = kzalloc(sizeof(buffer), GFP_KERNEL);
> > +	vmm = kzalloc(sizeof(buffer), GFP_KERNEL);
> 
> sizeof(buffer) == 4 or 8 - pretty short buffer...
> 
> > +
> > +	if (!element || !vmm || !vmc)
> > +		goto out_free;
> > +
> > +	wcount = count - copy_from_user(buffer, buf, count);
> > +	if (wcount < count)
> > +		goto out_free;
> 
> copy_from_user usage usually looks like:
> if (copy_from_user()) {
> 	ret = -EFAULT;
> 	goto err;
> }
> you don't seem to use copy_from_user return value for anything useful
> 
I did at one point, a few versions ago, that can likely be removed now.

> > +
> > +	i = sscanf(buffer, "%s %s %s", element, vmc, vmm);
> 
> what if user pass strings longer than size of buffers?
> 
You fail the write, theres a check at the top of the function for that.  By the
time you get here, buffer is guaranteed to be no more than 128 bytes.


> > +
> > +	if (i < 3)
> > +		goto out_free;
> > +
> > +	for (i = 0; i <= strlen(element); i++)
> > +		element[i] = tolower(element[i]);
> 
> it's harmless, but should be "i < strlen"
> 
Yeah, I guess we don't need a lower case \0 :)

> > +
> > +	if (!strncmp(vmc, "unlimited", 9))
> > +		new_rlim.rlim_cur = RLIM_INFINITY;
> > +	else
> > +		new_rlim.rlim_cur = simple_strtoull(vmc, NULL, 10);
> > +
> > +	if (!strncmp(vmm, "unlimited", 9))
> > +		new_rlim.rlim_max = RLIM_INFINITY;
> > +	else
> > +		new_rlim.rlim_max = simple_strtoull(vmm, NULL, 10);
> > +
> > +	for (i = 0; i < RLIM_NLIMITS; i++) {
> > +		if ((lnames[i].match) &&
> > +		    !strncmp(element, lnames[i].match,
> > +		     strlen(lnames[i].match))) {
> > +			index = i;
> > +			break;
> > +		}
> >  	}
> >  
> > +	if (!lock_task_sighand(task, &flags))
> > +		goto out_free;
> > +
> > +	if ((index >= 0) && (index < RLIM_NLIMITS))
> > +		do_setrlimit(index, &new_rlim, task);
> > +
> > +	unlock_task_sighand(task, &flags);
> > +
> > +out_free:
> > +	kfree(element);
> > +	kfree(vmc);
> > +	kfree(vmm);
> > +	kfree(buffer);
> > +out:
> > +	*ppos += count;
> > +	put_task_struct(task);
> >  	return count;
> >  }
> >  
> > (...)
> 


I'll make these corrections and repost.  Thanks!
Neil

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists