| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4ACE5A53.3010502@gmail.com>
Date: Thu, 08 Oct 2009 23:32:03 +0200
From: Marcin Slusarz <marcin.slusarz@...il.com>
To: Neil Horman <nhorman@...driver.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 (v4)
I found some new issues in this patch, sorry ;).
Neil Horman wrote:
> (...)
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6f742f6..631f01b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -49,6 +49,8 @@
>
> #include <asm/uaccess.h>
>
> +#include <linux/string.h>
> +#include <linux/ctype.h>
> #include <linux/errno.h>
> #include <linux/time.h>
> #include <linux/proc_fs.h>
> @@ -455,72 +457,193 @@ static int proc_oom_score(struct task_struct *task, char *buffer)
> struct limit_names {
> char *name;
> char *unit;
> + char *match;
> };
>
> static const struct limit_names lnames[RLIM_NLIMITS] = {
> - [RLIMIT_CPU] = {"Max cpu time", "ms"},
> - [RLIMIT_FSIZE] = {"Max file size", "bytes"},
> - [RLIMIT_DATA] = {"Max data size", "bytes"},
> - [RLIMIT_STACK] = {"Max stack size", "bytes"},
> - [RLIMIT_CORE] = {"Max core file size", "bytes"},
> - [RLIMIT_RSS] = {"Max resident set", "bytes"},
> - [RLIMIT_NPROC] = {"Max processes", "processes"},
> - [RLIMIT_NOFILE] = {"Max open files", "files"},
> - [RLIMIT_MEMLOCK] = {"Max locked memory", "bytes"},
> - [RLIMIT_AS] = {"Max address space", "bytes"},
> - [RLIMIT_LOCKS] = {"Max file locks", "locks"},
> - [RLIMIT_SIGPENDING] = {"Max pending signals", "signals"},
> - [RLIMIT_MSGQUEUE] = {"Max msgqueue size", "bytes"},
> - [RLIMIT_NICE] = {"Max nice priority", NULL},
> - [RLIMIT_RTPRIO] = {"Max realtime priority", NULL},
> - [RLIMIT_RTTIME] = {"Max realtime timeout", "us"},
> + [RLIMIT_CPU] = {"Max cpu time", "ms", "cpu"},
> + [RLIMIT_FSIZE] = {"Max file size", "bytes", "fsize"},
> + [RLIMIT_DATA] = {"Max data size", "bytes", "data"},
> + [RLIMIT_STACK] = {"Max stack size", "bytes", "stack"},
> + [RLIMIT_CORE] = {"Max core file size", "bytes", "core"},
> + [RLIMIT_RSS] = {"Max resident set", "bytes", "rss"},
> + [RLIMIT_NPROC] = {"Max processes", "processes", "nproc"},
> + [RLIMIT_NOFILE] = {"Max open files", "files", "nofile"},
> + [RLIMIT_MEMLOCK] = {"Max locked memory", "bytes", "memlock"},
> + [RLIMIT_AS] = {"Max address space", "bytes", "as"},
> + [RLIMIT_LOCKS] = {"Max file locks", "locks", "locks"},
> + [RLIMIT_SIGPENDING] = {"Max pending signals", "signals", "sigpending"},
> + [RLIMIT_MSGQUEUE] = {"Max msgqueue size", "bytes", "msgqueue"},
> + [RLIMIT_NICE] = {"Max nice priority", NULL, "nice"},
> + [RLIMIT_RTPRIO] = {"Max realtime priority", NULL, "rtprio"},
> + [RLIMIT_RTTIME] = {"Max realtime timeout", "us", "rttime"},
> };
There's no way user can figure out what's the "match" for every limit.
Maybe you could print it after "limit name"?
>
> /* 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 = -ENOMEM;
> + struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
>
> struct rlimit rlim[RLIM_NLIMITS];
>
> + bufptr = kzalloc(PAGE_SIZE, GFP_KERNEL);
I think you could derive size of allocation from RLIM_NLIMITS.
If I'm reading correctly it will be something like (RLIM_NLIMITS + 1) * 80.
> + if (!bufptr)
> + goto out;
> +
> + ccount = -EBUSY;
> +
> if (!lock_task_sighand(task, &flags))
> - return 0;
> + goto out_free;
> 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");
> }
>
> - return count;
> + ccount = -EMSGSIZE;
> +
> + if (*ppos >= bcount)
> + goto out_task;
> +
> + ccount = min(count, (size_t)(bcount-(*ppos)));
> + ccount = ccount - copy_to_user(buf, &bufptr[*ppos], ccount);
> + *ppos += ccount;
> +
> +out_task:
> + put_task_struct(task);
> +out_free:
> + kfree(bufptr);
> +out:
> + return ccount;
> +}
> +
> +#define PROC_PID_BUF_SZ 128
> +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 = -EMSGSIZE;
> + struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
> +
> + if (*ppos != 0)
> + goto out;
> +
> + if (count > PROC_PID_BUF_SZ)
> + goto out;
> +
> + wcount = -ENOMEM;
> + buffer = kzalloc(PROC_PID_BUF_SZ, GFP_KERNEL);
> +
> + if (!buffer)
> + goto out;
> +
> + element = kzalloc(PROC_PID_BUF_SZ, GFP_KERNEL);
> + vmc = kzalloc(PROC_PID_BUF_SZ, GFP_KERNEL);
> + vmm = kzalloc(PROC_PID_BUF_SZ, GFP_KERNEL);
> +
> + if (!element || !vmm || !vmc)
> + goto out_free;
> +
> + wcount = -EFAULT;
> + if (copy_from_user(buffer, buf, count))
> + goto out_free;
> +
> + i = sscanf(buffer, "%s %s %s", element, vmc, vmm);
> +
> + if (i < 3)
> + goto out_free;
> +
> + for (i = 0; i < strlen(element); i++)
> + element[i] = tolower(element[i]);
I don't think we should fix user mistakes like this...
> +
> + if (!strncmp(vmc, "unlimited", 9))
> + new_rlim.rlim_cur = RLIM_INFINITY;
> + else
> + new_rlim.rlim_cur = simple_strtoull(vmc, NULL, 10);
rlim_cur and rlim_max are unsigned long so you should use simple_strtoul
> +
> + 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) &&
match is always not null, you can drop this check
> + !strncmp(element, lnames[i].match,
> + strlen(lnames[i].match))) {
> + index = i;
> + break;
> + }
> + }
> +
> + wcount = -EBUSY;
> +
> + if (!lock_task_sighand(task, &flags))
> + goto out_free;
> +
> + wcount = -ENOENT;
> +
> + if ((index >= 0) && (index < RLIM_NLIMITS))
> + wcount = do_setrlimit(index, &new_rlim, task);
> +
> + unlock_task_sighand(task, &flags);
> +
> +out_free:
> + kfree(element);
> + kfree(vmc);
> + kfree(vmm);
> + kfree(buffer);
> +out:
> + if (!wcount) {
> + *ppos += count;
> + wcount = count;
> + }
> + put_task_struct(task);
> + return wcount;
> }
> (...)
--
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