| Message ID | 1266271241-6293-2-git-send-email-opurdila@ixiacom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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@ixiacom.com> > Cc: WANG Cong <amwang@redhat.com> > Cc: Eric W. Biederman <ebiederm@xmission.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 netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 February 2010 10:41:07 you wrote: > > + > > + 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? > OK, I'll give it a try. What about the EFAULT check, is that really required? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 February 2010 10:41:07 you wrote: > > +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. > I'm afraid we can't, skip_spaces does not accept userspace buffers. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Octavian Purdila wrote: > On Tuesday 16 February 2010 10:41:07 you wrote: > >>> + >>> + 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? >> > > OK, I'll give it a try. > > What about the EFAULT check, is that really required? I think so, it means to keep the errno to user-space when it is EFAULT, right? This seems reasonable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Octavian Purdila wrote: > On Tuesday 16 February 2010 10:41:07 you wrote: > >>> +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. >> > > I'm afraid we can't, skip_spaces does not accept userspace buffers. Well, you need to use copy_from_user() before call it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 February 2010 15:09:51 you wrote: > Octavian Purdila wrote: > > On Tuesday 16 February 2010 10:41:07 you wrote: > >>> +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. > > > > I'm afraid we can't, skip_spaces does not accept userspace buffers. > > Well, you need to use copy_from_user() before call it. > And how much would you copy? You need to either use a stack buffer and do a loop copy or you would need to copy the whole userspace buffer which means we need to allocate a kernel buffer. I think its much cleaner the way is currently done. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 February 2010 15:08:23 you wrote: > Octavian Purdila wrote: > > On Tuesday 16 February 2010 10:41:07 you wrote: > >>> + > >>> + 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? > > > > OK, I'll give it a try. > > > > What about the EFAULT check, is that really required? > > I think so, it means to keep the errno to user-space when it is EFAULT, > right? This seems reasonable. > The problem I see is that this way we don't actually acknowledge some of the set values, e.g. say that we have buffer="1 2 3" and length = 100. Although we do accept values 1, 2 and 3 we don't acknowledge that to the user (as we would do for, say "1 2 3 4a"), but return -EFAULT. I think it would be better to skip this check. That means that the user will get the ack for the 1, 2 and 3 values and next time it continues the write it will get -EFAULT. This will of course change the userspace ABI, albeit in a minor way, and it is not clear to me if doing this is allowed (even if this new approach would be the correct one). -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Octavian Purdila wrote: > On Tuesday 16 February 2010 15:09:51 you wrote: >> Octavian Purdila wrote: >>> On Tuesday 16 February 2010 10:41:07 you wrote: >>>>> +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. >>> I'm afraid we can't, skip_spaces does not accept userspace buffers. >> Well, you need to use copy_from_user() before call it. >> > > And how much would you copy? You need to either use a stack buffer and do a > loop copy or you would need to copy the whole userspace buffer which means we > need to allocate a kernel buffer. I think its much cleaner the way is currently > done. Yeah, maybe just a personal preference. :-/ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Octavian Purdila wrote: > On Tuesday 16 February 2010 15:08:23 you wrote: >> Octavian Purdila wrote: >>> On Tuesday 16 February 2010 10:41:07 you wrote: >>>>> + >>>>> + 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? >>> OK, I'll give it a try. >>> >>> What about the EFAULT check, is that really required? >> I think so, it means to keep the errno to user-space when it is EFAULT, >> right? This seems reasonable. >> > > The problem I see is that this way we don't actually acknowledge some of the > set values, e.g. say that we have buffer="1 2 3" and length = 100. Although we > do accept values 1, 2 and 3 we don't acknowledge that to the user (as we would > do for, say "1 2 3 4a"), but return -EFAULT. > > I think it would be better to skip this check. That means that the user will > get the ack for the 1, 2 and 3 values and next time it continues the write it > will get -EFAULT. > > This will of course change the userspace ABI, albeit in a minor way, and it is > not clear to me if doing this is allowed (even if this new approach would be > the correct one). > I think the right behavior is accept "1 2 3" and return the number of bytes that we accept. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Cong Wang <amwang@redhat.com> writes: > Octavian Purdila wrote: >> On Tuesday 16 February 2010 15:09:51 you wrote: >>> Octavian Purdila wrote: >>>> On Tuesday 16 February 2010 10:41:07 you wrote: >>>>>> +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. >>>> I'm afraid we can't, skip_spaces does not accept userspace buffers. >>> Well, you need to use copy_from_user() before call it. >>> >> >> And how much would you copy? You need to either use a stack buffer and do a >> loop copy or you would need to copy the whole userspace buffer which means we >> need to allocate a kernel buffer. I think its much cleaner the way is >> currently done. > > Yeah, maybe just a personal preference. :-/ There can be valid security reasons for copying all of the data before processing it. Semantically if we an guarantee that we either have processed the entire buffer or failed the entire buffer and no changes have occurred in the kernel that seems like a much easier semantic to work with in user space. Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 17 February 2010 15:31:45 you wrote: > >>> > >>> What about the EFAULT check, is that really required? > >> > >> I think so, it means to keep the errno to user-space when it is EFAULT, > >> right? This seems reasonable. > > > > The problem I see is that this way we don't actually acknowledge some of > > the set values, e.g. say that we have buffer="1 2 3" and length = 100. > > Although we do accept values 1, 2 and 3 we don't acknowledge that to the > > user (as we would do for, say "1 2 3 4a"), but return -EFAULT. > > > > I think it would be better to skip this check. That means that the user > > will get the ack for the 1, 2 and 3 values and next time it continues the > > write it will get -EFAULT. > > > > This will of course change the userspace ABI, albeit in a minor way, and > > it is not clear to me if doing this is allowed (even if this new approach > > would be the correct one). > > I think the right behavior is accept "1 2 3" and return the number of > bytes that we accept. > OK, it seems nobody is complaining about this corner case ABI change. I will remove the EFAULT check then. This will also help with making the code clearer, I hope. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 16 February 2010 09:48:56 you wrote: > On Tuesday 16 February 2010 10:41:07 you wrote: > > > + > > > + 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? > > OK, I'll give it a try. > After a couple of tries which didn't make it clearer, I've given up. Maybe its not clear, but is not too terribly messy IMO. I'll rather spend time on getting the new list range format done. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 17 February 2010 15:33:03 you wrote: > Cong Wang <amwang@redhat.com> writes: > > Octavian Purdila wrote: > >> On Tuesday 16 February 2010 15:09:51 you wrote: > >>> Octavian Purdila wrote: > >>>> On Tuesday 16 February 2010 10:41:07 you wrote: > >>>>>> +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. > >>>> > >>>> I'm afraid we can't, skip_spaces does not accept userspace buffers. > >>> > >>> Well, you need to use copy_from_user() before call it. > >> > >> And how much would you copy? You need to either use a stack buffer and > >> do a loop copy or you would need to copy the whole userspace buffer > >> which means we need to allocate a kernel buffer. I think its much > >> cleaner the way is currently done. > > > > Yeah, maybe just a personal preference. :-/ > > There can be valid security reasons for copying all of the data before > processing it. > How so? I only know about security issues when you copy & process the same data more than one time. And all existing code I've looked over (proc_dointvec, proc_dostring, bitmap_parse_user) does not copy all of the data. In fact the code in question here is just existing code moved to its own function. There must be a reason for doing that, as copying whole buffer seems like a much simple implementation? (my guess is to avoid an extra allocation) > Semantically if we an guarantee that we either have processed the > entire buffer or failed the entire buffer and no changes have occurred > in the kernel that seems like a much easier semantic to work with in > user space. > OK, but this is not how various proc routines currently handles it. For example proc_dointvec won't undo the changes for previous items when we get an error and I think for good reasons as I don't see a clean and generic way to do the undo stuff. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +} + +#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; + + *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; *lenp -= left; *ppos += *lenp; return 0; -#undef TMPBUFLEN } 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) { @@ -2237,8 +2274,8 @@ struct do_proc_dointvec_minmax_conv_param { 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; @@ -2251,7 +2288,7 @@ static int do_proc_dointvec_minmax_conv(int *negp, unsigned long *lvalp, } else { int val = *valp; if (val < 0) { - *negp = -1; + *negp = 1; *lvalp = (unsigned long)-val; } else { *negp = 0; @@ -2289,17 +2326,15 @@ int proc_dointvec_minmax(struct ctl_table *table, int write, } static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write, - void __user *buffer, + void __user *_buffer, size_t *lenp, loff_t *ppos, 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; + unsigned long *i, *min, *max; + int vleft, first = 1, err = 0; + size_t left; + char __user *buffer = (char __user *) _buffer; if (!data || !table->maxlen || !*lenp || (*ppos && !write)) { @@ -2314,82 +2349,37 @@ static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int left = *lenp; 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; + + err = proc_get_next_ulong(&buffer, &left, &val, &neg); + 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; - 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; - } - } - - 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)) + val = convdiv * (*i) / convmul; + err = proc_put_ulong(&buffer, &left, val, 0, 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; *lenp -= left; *ppos += *lenp; return 0; -#undef TMPBUFLEN } static int do_proc_doulongvec_minmax(struct ctl_table *table, int write, @@ -2450,7 +2440,7 @@ int proc_doulongvec_ms_jiffies_minmax(struct ctl_table *table, int write, } -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) { @@ -2462,7 +2452,7 @@ static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2473,7 +2463,7 @@ static int do_proc_dointvec_jiffies_conv(int *negp, unsigned long *lvalp, 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) { @@ -2485,7 +2475,7 @@ static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0; @@ -2496,7 +2486,7 @@ static int do_proc_dointvec_userhz_jiffies_conv(int *negp, unsigned long *lvalp, 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) { @@ -2506,7 +2496,7 @@ static int do_proc_dointvec_ms_jiffies_conv(int *negp, unsigned long *lvalp, int val = *valp; unsigned long lval; if (val < 0) { - *negp = -1; + *negp = 1; lval = (unsigned long)-val; } else { *negp = 0;
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@ixiacom.com> Cc: WANG Cong <amwang@redhat.com> Cc: Eric W. Biederman <ebiederm@xmission.com> --- kernel/sysctl.c | 298 +++++++++++++++++++++++++++---------------------------- 1 files changed, 144 insertions(+), 154 deletions(-)