| 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: <u2s412e6f7f1004301549tb0e88a80n4c621e42c0b31015@mail.gmail.com>
Date: Sat, 1 May 2010 06:49:59 +0800
From: Changli Gao <xiaosuo@...il.com>
To: Amerigo Wang <amwang@...hat.com>
Cc: linux-kernel@...r.kernel.org,
Octavian Purdila <opurdila@...acom.com>,
Eric Dumazet <eric.dumazet@...il.com>,
penguin-kernel@...ove.sakura.ne.jp, netdev@...r.kernel.org,
Neil Horman <nhorman@...driver.com>, ebiederm@...ssion.com,
David Miller <davem@...emloft.net>, adobriyan@...il.com
Subject: Re: [Patch 1/3] sysctl: refactor integer handling proc code
On Fri, Apr 30, 2010 at 4:25 PM, Amerigo Wang <amwang@...hat.com> wrote:
> (Based on Octavian's work, and I modified a lot.)
>
> As we are about to add another integer handling proc function a little
> bit of cleanup is in order: add a few helper functions to improve code
> readability and decrease code duplication.
>
> In the process a bug is also fixed: if the user specifies a number
> with more then 20 digits it will be interpreted as two integers
> (e.g. 10000...13 will be interpreted as 100.... and 13).
>
> Behavior for EFAULT handling was changed as well. Previous to this
> patch, when an EFAULT error occurred in the middle of a write
> operation, although some of the elements were set, that was not
> acknowledged to the user (by shorting the write and returning the
> number of bytes accepted). EFAULT is now treated just like any other
> errors by acknowledging the amount of bytes accepted.
>
> Signed-off-by: Octavian Purdila <opurdila@...acom.com>
> Signed-off-by: WANG Cong <amwang@...hat.com>
> Cc: Eric W. Biederman <ebiederm@...ssion.com>
> ---
>
> Index: linux-2.6/kernel/sysctl.c
> ===================================================================
> --- linux-2.6.orig/kernel/sysctl.c
> +++ linux-2.6/kernel/sysctl.c
> @@ -2040,8 +2040,122 @@ int proc_dostring(struct ctl_table *tabl
> buffer, lenp, ppos);
> }
>
> +static size_t proc_skip_spaces(char **buf)
> +{
> + size_t ret;
> + char *tmp = skip_spaces(*buf);
> + ret = tmp - *buf;
> + *buf = tmp;
> + return ret;
> +}
> +
> +#define TMPBUFLEN 22
> +/**
> + * proc_get_long - reads an ASCII formated integer from a user buffer
> + *
> + * @buf - a kernel buffer
> + * @size - size of the kernel buffer
> + * @val - this is where the number will be stored
> + * @neg - set to %TRUE if number is negative
> + * @perm_tr - a vector which contains the allowed trailers
> + * @perm_tr_len - size of the perm_tr vector
> + * @tr - pointer to store the trailer character
> + *
> + * In case of success 0 is returned and buf and size are updated with
> + * the amount of bytes read. If tr is non NULL and a trailing
> + * character exist (size is non zero after returning from this
> + * function) tr is updated with the trailing character.
> + */
> +static int proc_get_long(char **buf, size_t *size,
> + unsigned long *val, bool *neg,
> + const char *perm_tr, unsigned perm_tr_len, char *tr)
> +{
> + int len;
> + char *p, tmp[TMPBUFLEN];
> +
> + if (!*size)
> + return -EINVAL;
> +
> + len = *size;
> + if (len > TMPBUFLEN-1)
> + len = TMPBUFLEN-1;
> +
> + memcpy(tmp, *buf, len);
> +
> + tmp[len] = 0;
> + p = tmp;
> + if (*p == '-' && *size > 1) {
> + *neg = 1;
As neg is bool*, you should use true and false instead of 1 and 0.
> + p++;
> + } else
> + *neg = 0;
> + if (!isdigit(*p))
> + return -EINVAL;
> +
> + *val = simple_strtoul(p, &p, 0);
>
> -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp,
> + len = p - tmp;
> +
> + /* We don't know if the next char is whitespace thus we may accept
> + * invalid integers (e.g. 1234...a) or two integers instead of one
> + * (e.g. 123...1). So lets not allow such large numbers. */
> + if (len == TMPBUFLEN - 1)
> + return -EINVAL;
> +
> + if (len < *size && perm_tr_len && !memchr(perm_tr, *p, perm_tr_len))
> + return -EINVAL;
> +
> + if (tr && (len < *size))
> + *tr = *p;
> +
> + *buf += len;
> + *size -= len;
> +
> + return 0;
> +}
> +
> +/**
> + * proc_put_long - coverts an integer to a decimal ASCII formated string
> + *
> + * @buf - the user buffer
> + * @size - the size of the user buffer
> + * @val - the integer to be converted
> + * @neg - sign of the number, %TRUE for negative
> + *
> + * In case of success 0 is returned and buf and size are updated with
> + * the amount of bytes read.
> + */
> +static int proc_put_long(void __user **buf, size_t *size, unsigned long val,
> + bool neg)
> +{
> + int len;
> + char tmp[TMPBUFLEN], *p = tmp;
> +
> + sprintf(p, "%s%lu", neg ? "-" : "", val);
> + len = strlen(tmp);
> + if (len > *size)
> + len = *size;
> + if (copy_to_user(*buf, tmp, len))
> + return -EFAULT;
> + *size -= len;
> + *buf += len;
> + return 0;
> +}
> +#undef TMPBUFLEN
> +
> +static int proc_put_char(void __user **buf, size_t *size, char c)
> +{
> + if (*size) {
> + char __user **buffer = (char __user **)buf;
> + if (put_user(c, *buffer))
> + return -EFAULT;
> + (*size)--, (*buffer)++;
> + *buf = *buffer;
> + }
> + return 0;
> +}
> +
> +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
> int *valp,
> int write, void *data)
> {
> @@ -2050,7 +2164,7 @@ static int do_proc_dointvec_conv(int *ne
> } else {
> int val = *valp;
> if (val < 0) {
> - *negp = -1;
> + *negp = 1;
> *lvalp = (unsigned long)-val;
> } else {
> *negp = 0;
> @@ -2060,23 +2174,21 @@ static int do_proc_dointvec_conv(int *ne
> return 0;
> }
>
> +static const char proc_wspace_sep[] = { ' ', '\t', '\n' };
> +
> static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table,
> int write, void __user *buffer,
> size_t *lenp, loff_t *ppos,
> - int (*conv)(int *negp, unsigned long *lvalp, int *valp,
> + int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
> int write, void *data),
> void *data)
> {
> -#define TMPBUFLEN 21
> - int *i, vleft, first = 1, neg;
> - unsigned long lval;
> - size_t left, len;
> + int *i, vleft, first = 1, err = 0;
> + unsigned long page = 0;
> + size_t left;
> + char *kbuf;
>
> - char buf[TMPBUFLEN], *p;
> - char __user *s = buffer;
> -
> - if (!tbl_data || !table->maxlen || !*lenp ||
> - (*ppos && !write)) {
> + if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) {
> *lenp = 0;
> return 0;
> }
> @@ -2088,89 +2200,69 @@ static int __do_proc_dointvec(void *tbl_
> if (!conv)
> conv = do_proc_dointvec_conv;
>
> + if (write) {
> + if (left > PAGE_SIZE - 1)
> + left = PAGE_SIZE - 1;
> + page = __get_free_page(GFP_TEMPORARY);
> + kbuf = (char *) page;
> + if (!kbuf)
> + return -ENOMEM;
> + if (copy_from_user(kbuf, buffer, left)) {
> + err = -EFAULT;
> + goto free;
> + }
> + kbuf[left] = 0;
> + }
> +
> for (; left && vleft--; i++, first=0) {
> - if (write) {
> - while (left) {
> - char c;
> - if (get_user(c, s))
> - return -EFAULT;
> - if (!isspace(c))
> - break;
> - left--;
> - s++;
> - }
> - if (!left)
> - break;
> - neg = 0;
> - len = left;
> - if (len > sizeof(buf) - 1)
> - len = sizeof(buf) - 1;
> - if (copy_from_user(buf, s, len))
> - return -EFAULT;
> - buf[len] = 0;
> - p = buf;
> - if (*p == '-' && left > 1) {
> - neg = 1;
> - p++;
> - }
> - if (*p < '0' || *p > '9')
> - break;
> + unsigned long lval;
> + bool neg;
>
> - lval = simple_strtoul(p, &p, 0);
> + if (write) {
> + left -= proc_skip_spaces(&kbuf);
>
> - len = p-buf;
> - if ((len < left) && *p && !isspace(*p))
> + err = proc_get_long(&kbuf, &left, &lval, &neg,
> + proc_wspace_sep,
> + sizeof(proc_wspace_sep), NULL);
> + if (err)
> break;
> - s += len;
> - left -= len;
> -
> - if (conv(&neg, &lval, i, 1, data))
> + if (conv(&neg, &lval, i, 1, data)) {
> + err = -EINVAL;
> break;
> + }
> } else {
> - p = buf;
> + if (conv(&neg, &lval, i, 0, data)) {
> + err = -EINVAL;
> + break;
> + }
> if (!first)
> - *p++ = '\t';
> -
> - if (conv(&neg, &lval, i, 0, data))
> + err = proc_put_char(&buffer, &left, '\t');
> + if (err)
> + break;
> + err = proc_put_long(&buffer, &left, lval, neg);
> + if (err)
> break;
> -
> - sprintf(p, "%s%lu", neg ? "-" : "", lval);
> - len = strlen(buf);
> - if (len > left)
> - len = left;
> - if(copy_to_user(s, buf, len))
> - return -EFAULT;
> - left -= len;
> - s += len;
> }
> }
>
> - if (!write && !first && left) {
> - if(put_user('\n', s))
> - return -EFAULT;
> - left--, s++;
> - }
> + if (!write && !first && left && !err)
> + err = proc_put_char(&buffer, &left, '\n');
> + if (write && !err)
> + left -= proc_skip_spaces(&kbuf);
> +free:
> if (write) {
> - while (left) {
> - char c;
> - if (get_user(c, s++))
> - return -EFAULT;
> - if (!isspace(c))
> - break;
> - left--;
> - }
> + free_page(page);
> + if (first)
> + return err ? : -EINVAL;
> }
> - if (write && first)
> - return -EINVAL;
> *lenp -= left;
> *ppos += *lenp;
> - return 0;
> -#undef TMPBUFLEN
> + return err;
> }
>
> static int do_proc_dointvec(struct ctl_table *table, int write,
> void __user *buffer, size_t *lenp, loff_t *ppos,
> - int (*conv)(int *negp, unsigned long *lvalp, int *valp,
> + int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
> int write, void *data),
> void *data)
> {
> @@ -2238,8 +2330,8 @@ struct do_proc_dointvec_minmax_conv_para
> int *max;
> };
>
> -static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp,
> - int *valp,
> +static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> + int *valp,
> int write, void *data)
> {
> struct do_proc_dointvec_minmax_conv_param *param = data;
> @@ -2252,7 +2344,7 @@ static int do_proc_dointvec_minmax_conv(
> } else {
> int val = *valp;
> if (val < 0) {
> - *negp = -1;
> + *negp = 1;
> *lvalp = (unsigned long)-val;
> } else {
> *negp = 0;
> @@ -2295,102 +2387,78 @@ static int __do_proc_doulongvec_minmax(v
> unsigned long convmul,
> unsigned long convdiv)
> {
> -#define TMPBUFLEN 21
> - unsigned long *i, *min, *max, val;
> - int vleft, first=1, neg;
> - size_t len, left;
> - char buf[TMPBUFLEN], *p;
> - char __user *s = buffer;
> -
> - if (!data || !table->maxlen || !*lenp ||
> - (*ppos && !write)) {
> + unsigned long *i, *min, *max;
> + int vleft, first = 1, err = 0;
> + unsigned long page = 0;
> + size_t left;
> + char *kbuf;
> +
> + if (!data || !table->maxlen || !*lenp || (*ppos && !write)) {
> *lenp = 0;
> return 0;
> }
> -
> +
> i = (unsigned long *) data;
> min = (unsigned long *) table->extra1;
> max = (unsigned long *) table->extra2;
> vleft = table->maxlen / sizeof(unsigned long);
> left = *lenp;
> -
> +
> + if (write) {
> + if (left > PAGE_SIZE - 1)
> + left = PAGE_SIZE - 1;
> + page = __get_free_page(GFP_TEMPORARY);
> + kbuf = (char *) page;
> + if (!kbuf)
> + return -ENOMEM;
> + if (copy_from_user(kbuf, buffer, left)) {
> + err = -EFAULT;
> + goto free;
> + }
> + kbuf[left] = 0;
> + }
> +
> for (; left && vleft--; i++, min++, max++, first=0) {
> + unsigned long val;
> +
> if (write) {
> - while (left) {
> - char c;
> - if (get_user(c, s))
> - return -EFAULT;
> - if (!isspace(c))
> - break;
> - left--;
> - s++;
> - }
> - if (!left)
> - break;
> - neg = 0;
> - len = left;
> - if (len > TMPBUFLEN-1)
> - len = TMPBUFLEN-1;
> - if (copy_from_user(buf, s, len))
> - return -EFAULT;
> - buf[len] = 0;
> - p = buf;
> - if (*p == '-' && left > 1) {
> - neg = 1;
> - p++;
> - }
> - if (*p < '0' || *p > '9')
> - break;
> - val = simple_strtoul(p, &p, 0) * convmul / convdiv ;
> - len = p-buf;
> - if ((len < left) && *p && !isspace(*p))
> + bool neg;
> +
> + left -= proc_skip_spaces(&kbuf);
> +
> + err = proc_get_long(&kbuf, &left, &val, &neg,
> + proc_wspace_sep,
> + sizeof(proc_wspace_sep), NULL);
> + if (err)
> break;
> if (neg)
> - val = -val;
> - s += len;
> - left -= len;
> -
> - if(neg)
> continue;
> if ((min && val < *min) || (max && val > *max))
> continue;
> *i = val;
> } else {
> - p = buf;
> + val = convdiv * (*i) / convmul;
> if (!first)
> - *p++ = '\t';
> - sprintf(p, "%lu", convdiv * (*i) / convmul);
> - len = strlen(buf);
> - if (len > left)
> - len = left;
> - if(copy_to_user(s, buf, len))
> - return -EFAULT;
> - left -= len;
> - s += len;
> + err = proc_put_char(&buffer, &left, '\t');
> + err = proc_put_long(&buffer, &left, val, false);
> + if (err)
> + break;
> }
> }
>
> - if (!write && !first && left) {
> - if(put_user('\n', s))
> - return -EFAULT;
> - left--, s++;
> - }
> + if (!write && !first && left && !err)
> + err = proc_put_char(&buffer, &left, '\n');
> + if (write && !err)
> + left -= proc_skip_spaces(&kbuf);
> +free:
> if (write) {
> - while (left) {
> - char c;
> - if (get_user(c, s++))
> - return -EFAULT;
> - if (!isspace(c))
> - break;
> - left--;
> - }
> + free_page(page);
> + if (first)
> + return err ? : -EINVAL;
> }
> - if (write && first)
> - return -EINVAL;
> *lenp -= left;
> *ppos += *lenp;
> - return 0;
> -#undef TMPBUFLEN
> + return err;
> }
>
> static int do_proc_doulongvec_minmax(struct ctl_table *table, int write,
> @@ -2451,7 +2519,7 @@ int proc_doulongvec_ms_jiffies_minmax(st
> }
>
>
> -static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp,
> +static int do_proc_dointvec_jiffies_conv(bool *negp, unsigned long *lvalp,
> int *valp,
> int write, void *data)
> {
> @@ -2463,7 +2531,7 @@ static int do_proc_dointvec_jiffies_conv
> int val = *valp;
> unsigned long lval;
> if (val < 0) {
> - *negp = -1;
> + *negp = 1;
> lval = (unsigned long)-val;
> } else {
> *negp = 0;
> @@ -2474,7 +2542,7 @@ static int do_proc_dointvec_jiffies_conv
> return 0;
> }
>
> -static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp,
> +static int do_proc_dointvec_userhz_jiffies_conv(bool *negp, unsigned long *lvalp,
> int *valp,
> int write, void *data)
> {
> @@ -2486,7 +2554,7 @@ static int do_proc_dointvec_userhz_jiffi
> int val = *valp;
> unsigned long lval;
> if (val < 0) {
> - *negp = -1;
> + *negp = 1;
> lval = (unsigned long)-val;
> } else {
> *negp = 0;
> @@ -2497,7 +2565,7 @@ static int do_proc_dointvec_userhz_jiffi
> return 0;
> }
>
> -static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp,
> +static int do_proc_dointvec_ms_jiffies_conv(bool *negp, unsigned long *lvalp,
> int *valp,
> int write, void *data)
> {
> @@ -2507,7 +2575,7 @@ static int do_proc_dointvec_ms_jiffies_c
> int val = *valp;
> unsigned long lval;
> if (val < 0) {
> - *negp = -1;
> + *negp = 1;
> lval = (unsigned long)-val;
> } else {
> *negp = 0;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Regards，
Changli Gao(xiaosuo@...il.com)
Powered by blists - more mailing lists