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
| ||
|
Date: Tue, 16 Feb 2010 16:41:07 +0800 From: Cong Wang <amwang@...hat.com> To: Octavian Purdila <opurdila@...acom.com> CC: David Miller <davem@...emloft.net>, Linux Kernel Network Developers <netdev@...r.kernel.org>, Linux Kernel Developers <linux-kernel@...r.kernel.org>, "Eric W. Biederman" <ebiederm@...ssion.com> Subject: Re: [net-next PATCH v4 1/3] sysctl: refactor integer handling proc code Octavian Purdila wrote: > 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 fixed as well: 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). > > Signed-off-by: Octavian Purdila <opurdila@...acom.com> > Cc: WANG Cong <amwang@...hat.com> > Cc: Eric W. Biederman <ebiederm@...ssion.com> Some comments below. > --- > kernel/sysctl.c | 298 +++++++++++++++++++++++++++---------------------------- > 1 files changed, 144 insertions(+), 154 deletions(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 8a68b24..b0f9618 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2039,8 +2039,98 @@ int proc_dostring(struct ctl_table *table, int write, > buffer, lenp, ppos); > } > > +static int proc_skip_wspace(char __user **buf, size_t *size) > +{ > + char c; > + > + while (*size) { > + if (get_user(c, *buf)) > + return -EFAULT; > + if (!isspace(c)) > + break; > + (*size)--; (*buf)++; > + } > + > + return 0; > +} In lib/string.c we have skip_spaces(), I think we can use it here instead of inventing another one. > + > +#define TMPBUFLEN 22 > +static int proc_get_next_ulong(char __user **buf, size_t *size, > + unsigned long *val, bool *neg) > +{ > + int len; > + char *p, tmp[TMPBUFLEN]; > + int err; > + > + err = proc_skip_wspace(buf, size); > + if (err) > + return err; > + if (!*size) > + return -EINVAL; > + > + len = *size; > + if (len > TMPBUFLEN-1) > + len = TMPBUFLEN-1; > + > + if (copy_from_user(tmp, *buf, len)) > + return -EFAULT; > + > + tmp[len] = 0; > + p = tmp; > + if (*p == '-' && *size > 1) { > + *neg = 1; > + p++; > + } else > + *neg = 0; > + if (*p < '0' || *p > '9') > + return -EINVAL; isdigit(). > + > + *val = simple_strtoul(p, &p, 0); > + > + len = p - tmp; > + if (((len < *size) && *p && !isspace(*p)) || > + /* 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. */ > + len == TMPBUFLEN - 1) > + return -EINVAL; > > -static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, > + *buf += len; *size -= len; > + > + return 0; > +} > + > +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long val, > + bool neg, bool first) > +{ > + int len; > + char tmp[TMPBUFLEN], *p = tmp; > + > + if (!first) > + *p++ = '\t'; > + 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_newline(char __user **buf, size_t *size) > +{ > + if (*size) { > + if (put_user('\n', *buf)) > + return -EFAULT; > + (*size)--, (*buf)++; > + } > + return 0; > +} > + > +static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, > int *valp, > int write, void *data) > { > @@ -2049,7 +2139,7 @@ static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, > } else { > int val = *valp; > if (val < 0) { > - *negp = -1; > + *negp = 1; > *lvalp = (unsigned long)-val; > } else { > *negp = 0; > @@ -2060,19 +2150,15 @@ static int do_proc_dointvec_conv(int *negp, unsigned long *lvalp, > } > > static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, > - int write, void __user *buffer, > + 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; > - > - char buf[TMPBUFLEN], *p; > - char __user *s = buffer; > + int *i, vleft, first = 1, err = 0; > + size_t left; > + char __user *buffer = (char __user *) _buffer; > > if (!tbl_data || !table->maxlen || !*lenp || > (*ppos && !write)) { > @@ -2088,88 +2174,39 @@ static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, > conv = do_proc_dointvec_conv; > > 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; > - > - lval = simple_strtoul(p, &p, 0); > + unsigned long lval; > + bool neg; > > - len = p-buf; > - if ((len < left) && *p && !isspace(*p)) > + if (write) { > + err = proc_get_next_ulong(&buffer, &left, &lval, &neg); > + if (err) > break; > - s += len; > - left -= len; > - > if (conv(&neg, &lval, i, 1, data)) > break; > } else { > - p = buf; > - if (!first) > - *p++ = '\t'; > - > if (conv(&neg, &lval, i, 0, data)) > 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) { > - while (left) { > - char c; > - if (get_user(c, s++)) > - return -EFAULT; > - if (!isspace(c)) > + err = proc_put_ulong(&buffer, &left, lval, neg, first); > + if (err) > break; > - left--; > } > } > - if (write && first) > - return -EINVAL; > + > + if (!write && !first && left && !err) > + err = proc_put_newline(&buffer, &left); > + if (write && !err) > + err = proc_skip_wspace(&buffer, &left); > + if (err == -EFAULT /* do we really need to check for -EFAULT? */ || > + (write && first)) > + return err ? : -EINVAL; The logic here seems messy, adding one or two goto's may help? > *lenp -= left; > *ppos += *lenp; > return 0; > -#undef TMPBUFLEN > } > The rest looks fine. Thanks! -- 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