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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages
Loading