lib/bitmap.c:bitmap_parse() is a library function that received as input a user buffer. This seemed to have originated from the way the write_proc function of the /proc filesystem operates.
This function will be useful for other uses as well; for example, taking input for /sysfs instead of /proc, so it was changed to accept kernel buffers. We have this use for the Linux UWB project, as part as the upcoming bandwidth allocator code.
Only a few routines used this function and they were changed too.
Signed-off-by: Reinette Chatre <[email protected]>
---
Applies to the current Linus' kernel tip.
include/linux/bitmap.h | 4 ++--
include/linux/cpumask.h | 2 +-
include/linux/nodemask.h | 2 +-
kernel/irq/proc.c | 13 ++++++++++++-
kernel/profile.c | 13 ++++++++++++-
lib/bitmap.c | 15 +++++++--------
6 files changed, 35 insertions(+), 14 deletions(-)
diff -uprN -X linux-2.6.hg.vanilla/Documentation/dontdiff linux-2.6.hg.vanilla/include/linux/bitmap.h linux-2.6.hg/include/linux/bitmap.h
--- linux-2.6.hg.vanilla/include/linux/bitmap.h 2006-10-02 14:04:35.000000000 -0700
+++ linux-2.6.hg/include/linux/bitmap.h 2006-10-02 13:50:05.000000000 -0700
@@ -46,7 +46,7 @@
* bitmap_remap(dst, src, old, new, nbits) *dst = map(old, new)(src)
* bitmap_bitremap(oldbit, old, new, nbits) newbit = map(old, new)(oldbit)
* bitmap_scnprintf(buf, len, src, nbits) Print bitmap src to buf
- * bitmap_parse(ubuf, ulen, dst, nbits) Parse bitmap dst from user buf
+ * bitmap_parse(buf, len, dst, nbits) Parse bitmap dst from buf
* bitmap_scnlistprintf(buf, len, src, nbits) Print bitmap src as list to buf
* bitmap_parselist(buf, dst, nbits) Parse bitmap dst from list
* bitmap_find_free_region(bitmap, bits, order) Find and allocate bit region
@@ -106,7 +106,7 @@ extern int __bitmap_weight(const unsigne
extern int bitmap_scnprintf(char *buf, unsigned int len,
const unsigned long *src, int nbits);
-extern int bitmap_parse(const char __user *ubuf, unsigned int ulen,
+extern int bitmap_parse(const char *buf, unsigned int len,
unsigned long *dst, int nbits);
extern int bitmap_scnlistprintf(char *buf, unsigned int len,
const unsigned long *src, int nbits);
diff -uprN -X linux-2.6.hg.vanilla/Documentation/dontdiff linux-2.6.hg.vanilla/lib/bitmap.c linux-2.6.hg/lib/bitmap.c
--- linux-2.6.hg.vanilla/lib/bitmap.c 2006-10-02 14:04:39.000000000 -0700
+++ linux-2.6.hg/lib/bitmap.c 2006-10-02 13:51:57.000000000 -0700
@@ -317,8 +317,8 @@ EXPORT_SYMBOL(bitmap_scnprintf);
/**
* bitmap_parse - convert an ASCII hex string into a bitmap.
- * @ubuf: pointer to buffer in user space containing string.
- * @ubuflen: buffer size in bytes. If string is smaller than this
+ * @buf: pointer to buffer containing string.
+ * @buflen: buffer size in bytes. If string is smaller than this
* then it must be terminated with a \0.
* @maskp: pointer to bitmap array that will contain result.
* @nmaskbits: size of bitmap, in bits.
@@ -330,7 +330,7 @@ EXPORT_SYMBOL(bitmap_scnprintf);
* characters and for grouping errors such as "1,,5", ",44", "," and "".
* Leading and trailing whitespace accepted, but not embedded whitespace.
*/
-int bitmap_parse(const char __user *ubuf, unsigned int ubuflen,
+int bitmap_parse(const char *buf, unsigned int buflen,
unsigned long *maskp, int nmaskbits)
{
int c, old_c, totaldigits, ndigits, nchunks, nbits;
@@ -343,11 +343,10 @@ int bitmap_parse(const char __user *ubuf
chunk = ndigits = 0;
/* Get the next chunk of the bitmap */
- while (ubuflen) {
+ while (buflen) {
old_c = c;
- if (get_user(c, ubuf++))
- return -EFAULT;
- ubuflen--;
+ c = *buf++;
+ buflen--;
if (isspace(c))
continue;
@@ -388,7 +387,7 @@ int bitmap_parse(const char __user *ubuf
nbits += (nchunks == 1) ? nbits_to_hold_value(chunk) : CHUNKSZ;
if (nbits > nmaskbits)
return -EOVERFLOW;
- } while (ubuflen && c == ',');
+ } while (buflen && c == ',');
return 0;
}
diff -uprN -X linux-2.6.hg.vanilla/Documentation/dontdiff linux-2.6.hg.vanilla/include/linux/cpumask.h linux-2.6.hg/include/linux/cpumask.h
--- linux-2.6.hg.vanilla/include/linux/cpumask.h 2006-10-02 14:04:35.000000000 -0700
+++ linux-2.6.hg/include/linux/cpumask.h 2006-10-02 13:46:53.000000000 -0700
@@ -275,7 +275,7 @@ static inline int __cpumask_scnprintf(ch
#define cpumask_parse(ubuf, ulen, dst) \
__cpumask_parse((ubuf), (ulen), &(dst), NR_CPUS)
-static inline int __cpumask_parse(const char __user *buf, int len,
+static inline int __cpumask_parse(const char *buf, int len,
cpumask_t *dstp, int nbits)
{
return bitmap_parse(buf, len, dstp->bits, nbits);
diff -uprN -X linux-2.6.hg.vanilla/Documentation/dontdiff linux-2.6.hg.vanilla/kernel/irq/proc.c linux-2.6.hg/kernel/irq/proc.c
--- linux-2.6.hg.vanilla/kernel/irq/proc.c 2006-10-02 14:04:38.000000000 -0700
+++ linux-2.6.hg/kernel/irq/proc.c 2006-10-02 13:46:53.000000000 -0700
@@ -53,11 +53,22 @@ static int irq_affinity_write_proc(struc
{
unsigned int irq = (int)(long)data, full_count = count, err;
cpumask_t new_value, tmp;
+ char *kbuf;
+ unsigned long ret;
if (!irq_desc[irq].chip->set_affinity || no_irq_affinity)
return -EIO;
- err = cpumask_parse(buffer, count, new_value);
+ kbuf = kmalloc(count, GFP_KERNEL);
+ if (kbuf == NULL)
+ return -ENOMEM;
+ ret = copy_from_user(kbuf, buffer, count);
+ if (ret != 0) {
+ kfree(kbuf);
+ return -EFAULT;
+ }
+ err = cpumask_parse(kbuf, count, new_value);
+ kfree(kbuf);
if (err)
return err;
diff -uprN -X linux-2.6.hg.vanilla/Documentation/dontdiff linux-2.6.hg.vanilla/include/linux/nodemask.h linux-2.6.hg/include/linux/nodemask.h
--- linux-2.6.hg.vanilla/include/linux/nodemask.h 2006-10-02 14:04:36.000000000 -0700
+++ linux-2.6.hg/include/linux/nodemask.h 2006-10-02 13:46:53.000000000 -0700
@@ -290,7 +290,7 @@ static inline int __nodemask_scnprintf(c
#define nodemask_parse(ubuf, ulen, dst) \
__nodemask_parse((ubuf), (ulen), &(dst), MAX_NUMNODES)
-static inline int __nodemask_parse(const char __user *buf, int len,
+static inline int __nodemask_parse(const char *buf, int len,
nodemask_t *dstp, int nbits)
{
return bitmap_parse(buf, len, dstp->bits, nbits);
diff -uprN -X linux-2.6.hg.vanilla/Documentation/dontdiff linux-2.6.hg.vanilla/kernel/profile.c linux-2.6.hg/kernel/profile.c
--- linux-2.6.hg.vanilla/kernel/profile.c 2006-10-02 14:04:38.000000000 -0700
+++ linux-2.6.hg/kernel/profile.c 2006-10-02 13:46:53.000000000 -0700
@@ -395,8 +395,19 @@ static int prof_cpu_mask_write_proc (str
cpumask_t *mask = (cpumask_t *)data;
unsigned long full_count = count, err;
cpumask_t new_value;
+ char *kbuf;
+ unsigned long ret;
- err = cpumask_parse(buffer, count, new_value);
+ kbuf = kmalloc(count, GFP_KERNEL);
+ if (kbuf == NULL)
+ return -ENOMEM;
+ ret = copy_from_user(kbuf, buffer, count);
+ if (ret != 0) {
+ kfree(kbuf);
+ return -EFAULT;
+ }
+ err = cpumask_parse(kbuf, count, new_value);
+ kfree(kbuf);
if (err)
return err;
Hi Andrew
We are going to use this in the Linux UWB project; is it
possible/makes it sense to get it in?
> lib/bitmap.c:bitmap_parse() is a library function that received as input a
> user buffer. This seemed to have originated from the way the write_proc
> function of the /proc filesystem operates.
>
> This function will be useful for other uses as well; for example, taking
> input for /sysfs instead of /proc, so it was changed to accept kernel
> buffers. We have this use for the Linux UWB project, as part as the
> upcoming bandwidth allocator code.
>
> Only a few routines used this function and they were changed too.
>
> Signed-off-by: Reinette Chatre <[email protected]>
Signed-off-by: Inaky Perez-Gonzalez <[email protected]>
-- Inaky
On Tue, 3 Oct 2006 08:16:27 -0700
Reinette Chatre <[email protected]> wrote:
> lib/bitmap.c:bitmap_parse() is a library function that received as input a user buffer. This seemed to have originated from the way the write_proc function of the /proc filesystem operates.
>
> This function will be useful for other uses as well; for example, taking input for /sysfs instead of /proc, so it was changed to accept kernel buffers. We have this use for the Linux UWB project, as part as the upcoming bandwidth allocator code.
>
> Only a few routines used this function and they were changed too.
Fair enough. But this:
>
> - err = cpumask_parse(buffer, count, new_value);
> + kbuf = kmalloc(count, GFP_KERNEL);
> + if (kbuf == NULL)
> + return -ENOMEM;
> + ret = copy_from_user(kbuf, buffer, count);
> + if (ret != 0) {
> + kfree(kbuf);
> + return -EFAULT;
> + }
> + err = cpumask_parse(kbuf, count, new_value);
> + kfree(kbuf);
> if (err)
> return err;
>
> ...
>
> - err = cpumask_parse(buffer, count, new_value);
> + kbuf = kmalloc(count, GFP_KERNEL);
> + if (kbuf == NULL)
> + return -ENOMEM;
> + ret = copy_from_user(kbuf, buffer, count);
> + if (ret != 0) {
> + kfree(kbuf);
> + return -EFAULT;
> + }
> + err = cpumask_parse(kbuf, count, new_value);
> + kfree(kbuf);
> if (err)
> return err;
>
is sending us a message ;)
How about adding a new bitmap_parse_user() (and cpumask_parse_user()) which
does the above?
I believe that bitmap_parse() was contributed by Joe Korty.
Let's add him to this cc list too.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401
On Tue, Oct 03, 2006 at 04:39:36PM -0700, Andrew Morton wrote:
> On Tue, 3 Oct 2006 08:16:27 -0700
> Reinette Chatre <[email protected]> wrote:
>
> > lib/bitmap.c:bitmap_parse() is a library function that received as
> > input a user buffer. This seemed to have originated from the way the
> > write_proc function of the /proc filesystem operates.
> >
> > This function will be useful for other uses as well;
> > for example, taking input for /sysfs instead of /proc,
> > so it was changed to accept kernel buffers. We have this
> > use for the Linux UWB project, as part as the upcoming
> > bandwidth allocator code.
> >
> > Only a few routines used this function and they were changed too.
>
> Fair enough. But this: [ ... ] is sending us a message ;)
>
> How about adding a new bitmap_parse_user() (and cpumask_parse_user())
> which does the above?
I am slightly concerned about using a kmalloc where 'count' is specified
by userspace. There might be a DoS attack in here somewhere.....
Perhaps we can reverse Andrew's idea: rename the existing bitmap_parse
to bitmap_parse_user, then make the kernel-buffer version, bitmap_parse,
be a wrapper around that.
Joe
> I am slightly concerned about using a kmalloc where 'count' is specified
> by userspace. There might be a DoS attack in here somewhere.....
Good point. One should usually guard such a kmalloc, by checking the
count from user space against some crude upper limit, that is big
enough for any legitimate purposes, but avoids trying to allocate some
humongous amount. For example, see kernel/cpuset.c:
/* Crude upper limit on largest legitimate cpulist user might write. */
if (nbytes > 100 + 6 * NR_CPUS)
return -E2BIG;
> Perhaps we can reverse Andrew's idea: rename the existing bitmap_parse
> to bitmap_parse_user, then make the kernel-buffer version, bitmap_parse,
> be a wrapper around that.
Perhaps I should have my coffee first, but I don't see where the
order in which we wrap these affects the need to impose a crude
upper limit on what the user can ask for.
Off hand, I'd expect the kernel version to be the actual implementing
code, and the user version to be the wrapper and also to impose the
crude upper limit.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401
On Wed, Oct 04, 2006 at 07:27:46AM -0700, Paul Jackson wrote:
> Perhaps I should have my coffee first, but I don't see where the
> order in which we wrap these affects the need to impose a crude
> upper limit on what the user can ask for.
>
> Off hand, I'd expect the kernel version to be the actual implementing
> code, and the user version to be the wrapper and also to impose the
> crude upper limit.
I guess I am a sucker for no-transient-buffer (bufferless?)
implementations, as with them there is an intrinsic
simplicity that automatically avoids problems. The price
in this case, though, is the use of the more expensive
get_user() where, for kernel buffers, it is not needed.
I have no objection though, and in either case we should
impose a sanity check on 'count'.
Joe
Joe wrote:
> I guess I am a sucker for no-transient-buffer (bufferless?)
Ah - that explains Joe's preference for putting the actual implementing
code in the user version - it gets to pull in the user string one
char at a time, avoiding a malloc'd buffer.
I tend to make more coding mistakes that way, so have gotten in the
habit of keeping my scanning code separate from any code required to
get a nice safe local copy of the input that is to be scanned.
I agree with Joe - either way can be made to work - author's
discretion. Just be sure to impose that sanity limit.
--
I won't rest till it's the best ...
Programmer, Linux Scalability
Paul Jackson <[email protected]> 1.925.600.0401
> Joe wrote:
>> I guess I am a sucker for no-transient-buffer (bufferless?)
>
> Ah - that explains Joe's preference for putting the actual implementing
> code in the user version - it gets to pull in the user string one
> char at a time, avoiding a malloc'd buffer.
I tend to gree w/ Joe there.
I wonder if a hybrid would be ok, although I the pseudo impl
I propose below is kind of dirty, but some people might find it
justified enough:
static
__bitmap_parse(const void *_buf, size_t size, enum { KERNEL, USER } type,
unsigned long *dst, int nbits)
{
const char __user *ubuf = _buf;
const char *buf = _buf;
...
switch(type) {
case USER:
if (get_user(c, ubuf++))
return -EFAULT;
break;
case KERNEL:
c = *buf++;
break;
default:
BUG();
...
}
int bitmap_parse(const char *buf, unsigned int buflen,
unsigned long *maskp, int nmaskbits) {
return __bitmap_parse(buf, buflen, KERNEL, maskp, nmaskbits);
}
int bitmap_parse_user(const char __user *buf, unsigned int buflen,
unsigned long *maskp, int nmaskbits) {
return __bitmap_parse(buf, buflen, USER, maskp, nmaskbits);
}
[that or exposing __bitmap_user() as a extern / EXPORT and putting
bitmap_parse{,_user}() as an inline in the header file]
It's nitty-gritty, but it removes the kmalloc from the equation...
--
Inaky
On Wed, 4 Oct 2006 10:14:05 -0400
Joe Korty <[email protected]> wrote:
> On Tue, Oct 03, 2006 at 04:39:36PM -0700, Andrew Morton wrote:
> > On Tue, 3 Oct 2006 08:16:27 -0700
> > Reinette Chatre <[email protected]> wrote:
> >
> > > lib/bitmap.c:bitmap_parse() is a library function that received as
> > > input a user buffer. This seemed to have originated from the way the
> > > write_proc function of the /proc filesystem operates.
> > >
> > > This function will be useful for other uses as well;
> > > for example, taking input for /sysfs instead of /proc,
> > > so it was changed to accept kernel buffers. We have this
> > > use for the Linux UWB project, as part as the upcoming
> > > bandwidth allocator code.
> > >
> > > Only a few routines used this function and they were changed too.
> >
> > Fair enough. But this: [ ... ] is sending us a message ;)
> >
> > How about adding a new bitmap_parse_user() (and cpumask_parse_user())
> > which does the above?
>
>
> I am slightly concerned about using a kmalloc where 'count' is specified
> by userspace. There might be a DoS attack in here somewhere.....
spose so. It would allow an unprivileged app to force lots of order-3
allocations, which can make page reclaim do a lot of work. Still...
> Perhaps we can reverse Andrew's idea: rename the existing bitmap_parse
> to bitmap_parse_user, then make the kernel-buffer version, bitmap_parse,
> be a wrapper around that.
>
I think we can do a version which omits the kmalloc altogether:
/*
* insert suitable comment here
*/
int bitmap_parse_kernel(const char *ubuf, unsigned int ubuflen,
unsigned long *maskp, int nmaskbits)
{
int c, old_c, totaldigits, ndigits, nchunks, nbits;
u32 chunk;
bitmap_zero(maskp, nmaskbits);
nchunks = nbits = totaldigits = c = 0;
do {
chunk = ndigits = 0;
/* Get the next chunk of the bitmap */
while (ubuflen) {
old_c = c;
if (__get_user(c, ubuf++))
return -EFAULT;
(Note the s/get_user/__get_user/)
int bitmap_parse_user(const char __user *ubuf, unsigned int ubuflen,
unsigned long *maskp, int nmaskbits)
{
if (!access_ok(VERIFY_READ, ubuf, ubuflen))
return -EFAULT;
return bitmap_parse_kernel((const char *)ubuf, ubuflen,
maskp, nmaskbits);
}
(Does typecasting a __user char * to a char * make sparse happy?)
On Wed, Oct 04, 2006 at 09:40:29AM -0700, Andrew Morton wrote:
> I think we can do a version which omits the kmalloc altogether:
> if (__get_user(c, ubuf++))
> return -EFAULT;
>
> (Note the s/get_user/__get_user/)
Nice. This eliminates the bulk of the get_user() overhead. And
it can be merged into Inaky's enum solution too, for something
that is both simple and efficient.
Joe
Joe Korty wrote:
> On Wed, Oct 04, 2006 at 09:40:29AM -0700, Andrew Morton wrote:
>> I think we can do a version which omits the kmalloc altogether:
>
>> if (__get_user(c, ubuf++))
>> return -EFAULT;
>>
>> (Note the s/get_user/__get_user/)
>
> Nice. This eliminates the bulk of the get_user() overhead. And
> it can be merged into Inaky's enum solution too, for something
> that is both simple and efficient.
Then bitmap_parse_user() will need a quick access_ok() check
before calling down on __bitmap_parse(). Nice.
--
Inaky