Messages in this thread | | | Date | Sun, 04 Oct 2009 22:04:20 +0200 | From | Marcin Slusarz <> | Subject | Re: [PATCH 1/3] extend get/setrlimit to support setting rlimits external to a process (v3) |
| |
Neil Horman wrote: >>> +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.
But you allocated only 4/8 bytes (depending on size of void*) for element, vmc, vmm. It will overflow for string like "xxxxxxxxxxxxxxxxxxx y z".
Marcin
| |