kmemdup_from_user(): introduce

I notice there are many places doing copy_from_user() which follows kmalloc():

       dst = kmalloc(len, GFP_KERNEL);
       if (!dst)
               return -ENOMEM;
       if (copy_from_user(dst, src, len)) {
		kfree(dst);
		return -EFAULT
	}

kmemdup_from_user() is a wrapper of the above code. With this new function, we don't have to write 'len' twice, which can lead to typos/mistakes. It also produces smaller code.

A qucik grep shows 250+ places where kmemdup_from_user() *may* be used. I'll prepare a patchset to do this conversion.

Signed-off-by Li Zefan <[email protected]>

---
include/linux/string.h | 1 +
mm/util.c | 24 ++++++++++++++++++++++++

2 files changed, 25 insertions(+), 0 deletions(-)

include/linux/string.h +1 -0

index 76ec218..397e622 100644
@@ -105,6 +105,7 @@ extern void * memchr(const void *,int,__kernel_size_t);
 extern char *kstrdup(const char *s, gfp_t gfp);
 extern char *kstrndup(const char *s, size_t len, gfp_t gfp);
 extern void *kmemdup(const void *src, size_t len, gfp_t gfp);
+extern void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp);
 
 extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
 extern void argv_free(char **argv);

mm/util.c +1 -0

index 37eaccd..a608ebb 100644
@@ -70,6 +70,30 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
 EXPORT_SYMBOL(kmemdup);
 
 /**
+ * kmemdup_from_user - duplicate memory region from user space

mm/util.c +13 -0

+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(p, src, len)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return p;
+}
+EXPORT_SYMBOL(kmemdup_from_user);
+
+/**
 * __krealloc - like krealloc() but don't free @p.
 * @p: object to reallocate memory for.
 * @new_size: how many bytes of memory are required.

Signature · 6 lines

1.5.4.rc3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 8 lines

/**
+ * kmemdup_from_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ */
+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)

mm/util.c void *kmemdup(const void *src, size_t len, gfp_t gfp) · line 83

Quoted 4 lines

mm/util.c +4 -0

+{
+	void *p;
+
+	p = kmalloc_track_caller(len, gfp);

Well, you use kmalloc_track_caller, instead of kmalloc as you showed above. :) Why don't you mention this?

Quoted 8 lines

Quoted 8 lines

/**
+ * kmemdup_from_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ */
+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+{
+	void *p;
+
+	p = kmalloc_track_caller(len, gfp);

Well, you use kmalloc_track_caller, instead of kmalloc as you showed above. :) Why don't you mention this?

kmalloc() wrapper function must use kmalloc_track_caller().

mm/util.c void *kmemdup(const void *src, size_t len, gfp_t gfp) · line 83

Quoted 5 lines

mm/util.c +5 -0

+void *kmemdup_from_user(const void __user *src, size_t len, gfp_t gfp)
+{
+	void *p;
+
+	p = kmalloc_track_caller(len, gfp);

Well, you use kmalloc_track_caller, instead of kmalloc as you showed above. :) Why don't you mention this?

I notice there are many places doing copy_from_user() which follows kmalloc():

        dst = kmalloc(len, GFP_KERNEL);
        if (!dst)
                return -ENOMEM;
        if (copy_from_user(dst, src, len)) {
		kfree(dst);
		return -EFAULT
	}

kmemdup_from_user() is a wrapper of the above code. With this new function, we don't have to write 'len' twice, which can lead to typos/mistakes. It also produces smaller code.

A qucik grep shows 250+ places where kmemdup_from_user() *may* be used. I'll prepare a patchset to do this conversion.

Quoted 11 lines · Li Zefan

I notice there are many places doing copy_from_user() which follows kmalloc():

        dst = kmalloc(len, GFP_KERNEL);
        if (!dst)
                return -ENOMEM;
        if (copy_from_user(dst, src, len)) {
		kfree(dst);
		return -EFAULT
	}

Quoted 3 lines

kmemdup_from_user() is a wrapper of the above code. With this new function, we don't have to write 'len' twice, which can lead to typos/mistakes. It also produces smaller code.

Name totally sucks, it mixes kernel idiom of allocation with purely userspace function.

Quoted 2 lines

A qucik grep shows 250+ places where kmemdup_from_user() *may* be used. I'll prepare a patchset to do this conversion.

250?

Let's not add wrapper for every two lines that happen to be used together.

BTW, can we drop strstarts() and kzfree() on the same reasoning?

Quoted 3 lines

Let's not add wrapper for every two lines that happen to be used together.

Why not if we have good reasons? And I don't think we can call this "happen to" if there are 250+ of them?

Quoted 6 lines · On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:

Quoted 3 lines

Let's not add wrapper for every two lines that happen to be used together.

Why not if we have good reasons? And I don't think we can call this "happen to" if there are 250+ of them?

The change is a good one. If a reviewer (me) sees it then you know the code's all right and the review effort becomes less - all you need to check is that the call site is using IS_ERR/PTR_ERR and isn't testing for NULL. Less code, less chance for bugs.

Plus it makes kernel text smaller.

Yes, the name is a bit cumbersome.

Quoted 14 lines · On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:

Quoted 7 lines

Quoted 4 lines

Let's not add wrapper for every two lines that happen to be used together.

Why not if we have good reasons? And I don't think we can call this "happen to" if there are 250+ of them?

The change is a good one. If a reviewer (me) sees it then you know the code's all right and the review effort becomes less - all you need to check is that the call site is using IS_ERR/PTR_ERR and isn't testing for NULL. Less code, less chance for bugs.

Quoted 3 lines

Plus it makes kernel text smaller.

Yes, the name is a bit cumbersome.

Some do NUL-termination afterwards and allocate "len + 1", but copy "len". Some don't care.

Quoted 5 lines · On Fri, 06 Mar 2009 16:27:53 +0800 Li Zefan <[email protected]> wrote:

Quoted 3 lines

Let's not add wrapper for every two lines that happen to be used together.

Why not if we have good reasons? And I don't think we can call this "happen to" if there are 250+ of them?

The change is a good one. If a reviewer (me) sees it then you know the code's all right and the review effort becomes less - all you need to check is that the call site is using IS_ERR/PTR_ERR and isn't testing for NULL. Less code, less chance for bugs.

Plus it makes kernel text smaller.

Yes, the name is a bit cumbersome.

How about memdup_user()? like kstrndup() vs strndup_user().

I notice there are many places doing copy_from_user() which follows kmalloc():

        dst = kmalloc(len, GFP_KERNEL);
        if (!dst)
                return -ENOMEM;
        if (copy_from_user(dst, src, len)) {
		kfree(dst);
		return -EFAULT
	}

memdup_user() is a wrapper of the above code. With this new function, we don't have to write 'len' twice, which can lead to typos/mistakes. It also produces smaller code and kernel text.

A quick grep shows 250+ places where memdup_user() *may* be used. I'll prepare a patchset to do this conversion.

v1 -> v2: change the name from kmemdup_from_user to memdup_user.

Signed-off-by Li Zefan <[email protected]>

---

Can this get into 2.6.29, so I can prepare patches based on linux-next? And this won't cause regression, since no one uses it yet. :)

include/linux/string.h +1 -0

index 76ec218..79f30f3 100644
@@ -12,6 +12,7 @@
 #include <linux/stddef.h>	/* for NULL */
 
 extern char *strndup_user(const char __user *, long);
+extern void *memdup_user(const void __user *, size_t, gfp_t);
 
 /*
  * Include machine specific inline routines

mm/util.c +26 -0

index 37eaccd..3d21c21 100644
@@ -70,6 +70,32 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
 EXPORT_SYMBOL(kmemdup);
 
 /**
+ * memdup_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ *
+ * Returns an ERR_PTR() on failure.
+ */
+void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
+{
+	void *p;
+
+	p = kmalloc_track_caller(len, gfp);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	if (copy_from_user(p, src, len)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return p;
+}
+EXPORT_SYMBOL(memdup_user);
+
+/**
  * __krealloc - like krealloc() but don't free @p.
  * @p: object to reallocate memory for.
  * @new_size: how many bytes of memory are required.

Signature · 1 line

1.5.4.rc3

Quoted 18 lines

/**
+ * memdup_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ *
+ * Returns an ERR_PTR() on failure.
+ */

mm/util.c +8 -0

+void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
+{
+	void *p;
+
+	p = kmalloc_track_caller(len, gfp);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+

mm/util.c void *kmemdup(const void *src, size_t len, gfp_t gfp) · line 96

Quoted 8 lines

mm/util.c +8 -0

+	if (copy_from_user(p, src, len)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return p;
+}
+EXPORT_SYMBOL(memdup_user);

I would like to question the use of the gfp argument here; copy_from_user sleeps, so you can't use GFP_ATOMIC anyway. You can't use GFP_NOFS etc, because the pagefault path will happily do things that are equivalent, if not identical, to GFP_KERNEL.

Quoted 18 lines · On Fri, 6 Mar 2009 15:03:35 -0800 Andrew Morton <[email protected]>…

Quoted 18 lines

/**
+ * memdup_user - duplicate memory region from user space
+ *
+ * @src: source address in user space
+ * @len: number of bytes to copy
+ * @gfp: GFP mask to use
+ *
+ * Returns an ERR_PTR() on failure.
+ */

mm/util.c +8 -0

+void *memdup_user(const void __user *src, size_t len, gfp_t gfp)
+{
+	void *p;
+
+	p = kmalloc_track_caller(len, gfp);
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+

Quoted 8 lines

mm/util.c void *kmemdup(const void *src, size_t len, gfp_t gfp) · line 96

Quoted 8 lines

mm/util.c +8 -0

+	if (copy_from_user(p, src, len)) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+
+	return p;
+}
+EXPORT_SYMBOL(memdup_user);

Hi,

I like the general idea of this a lot; it will make things much less error prone (and we can add some sanity checks on "len" to catch the standard security holes around copy_from_user usage). I'd even also want a memdup_array() like thing in the style of calloc().

However, I have two questions/suggestions for improvement:

I would like to question the use of the gfp argument here; copy_from_user sleeps, so you can't use GFP_ATOMIC anyway. You can't use GFP_NOFS etc, because the pagefault path will happily do things that are equivalent, if not identical, to GFP_KERNEL.

So the only value you can pass in correctly, as far as I can see, is GFP_KERNEL. Am I wrong?

A second thing.. I'd like to have this function return NULL on failure; error checking a pointer for NULL is so much easier than testing for anything else; the only distinction is -ENOMEM versus -EFAULT, and I'm not sure that that is worth the complexity on all callers.

Quoted 1 line

Quoted 1 line

+EXPORT_SYMBOL(memdup_user);

Hi,

I like the general idea of this a lot; it will make things much less error prone (and we can add some sanity checks on "len" to catch the standard security holes around copy_from_user usage). I'd even also want a memdup_array() like thing in the style of calloc().

However, I have two questions/suggestions for improvement:

I would like to question the use of the gfp argument here; copy_from_user sleeps, so you can't use GFP_ATOMIC anyway. You can't use GFP_NOFS etc, because the pagefault path will happily do things that are equivalent, if not identical, to GFP_KERNEL.

So the only value you can pass in correctly, as far as I can see, is GFP_KERNEL. Am I wrong?

Quoted 16 lines

Quoted 1 line

Quoted 1 line

+EXPORT_SYMBOL(memdup_user);

Hi,

I like the general idea of this a lot; it will make things much less error prone (and we can add some sanity checks on "len" to catch the standard security holes around copy_from_user usage). I'd even also want a memdup_array() like thing in the style of calloc().

However, I have two questions/suggestions for improvement:

I would like to question the use of the gfp argument here; copy_from_user sleeps, so you can't use GFP_ATOMIC anyway. You can't use GFP_NOFS etc, because the pagefault path will happily do things that are equivalent, if not identical, to GFP_KERNEL.

Quoted 3 lines

So the only value you can pass in correctly, as far as I can see, is GFP_KERNEL. Am I wrong?

Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(), so we have one more reason to use this memdup_user().

Quoted 15 lines · On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <[email protected]> wrote:

Quoted 15 lines

Quoted 1 line

Quoted 1 line

+EXPORT_SYMBOL(memdup_user);

Hi,

I like the general idea of this a lot; it will make things much less error prone (and we can add some sanity checks on "len" to catch the standard security holes around copy_from_user usage). I'd even also want a memdup_array() like thing in the style of calloc().

However, I have two questions/suggestions for improvement:

I would like to question the use of the gfp argument here; copy_from_user sleeps, so you can't use GFP_ATOMIC anyway. You can't use GFP_NOFS etc, because the pagefault path will happily do things that are equivalent, if not identical, to GFP_KERNEL.

Quoted 5 lines

Quoted 3 lines

So the only value you can pass in correctly, as far as I can see, is GFP_KERNEL. Am I wrong?

Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(), so we have one more reason to use this memdup_user().

gack, those callsites are probably buggy. Where are they?

Quoted 17 lines · Andrew Morton

Quoted 15 lines · On Mon, 09 Mar 2009 10:22:08 +0800 Li Zefan <[email protected]> wrote:

Quoted 15 lines

Quoted 1 line

Quoted 1 line

+EXPORT_SYMBOL(memdup_user);

Hi,

I like the general idea of this a lot; it will make things much less error prone (and we can add some sanity checks on "len" to catch the standard security holes around copy_from_user usage). I'd even also want a memdup_array() like thing in the style of calloc().

However, I have two questions/suggestions for improvement:

I would like to question the use of the gfp argument here; copy_from_user sleeps, so you can't use GFP_ATOMIC anyway. You can't use GFP_NOFS etc, because the pagefault path will happily do things that are equivalent, if not identical, to GFP_KERNEL.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

So the only value you can pass in correctly, as far as I can see, is GFP_KERNEL. Am I wrong?

Right! I just dug and found a few kmalloc(GFP_ATOMIC/GFP_NOFS)+copy_from_user(), so we have one more reason to use this memdup_user().

gack, those callsites are probably buggy. Where are they?

Yes, either buggy or should use GFP_KERNEL.

All are in -mm only, except the first one:

drivers/isdn/i4l/isdn_common.c:

	struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
	...
	if (copy_from_user(skb_put(skb, len), buf, len)) {

net/irda/af_irda.c:

	ias_opt = kmalloc(sizeof(struct irda_ias_set), GFP_ATOMIC);
	...
	if (copy_from_user(ias_opt, optval, optlen)) {

fs/btrfs/ioctl.c:

	vol_args = kmalloc(sizeof(*vol_args), GFP_NOFS);
	...
	if (copy_from_user(vol_args, arg, sizeof(*vol_args))) {

Quoted 1 line

fs/ocfs2/dlm/dlmfs.c:

	lvb_buf = kmalloc(writelen, GFP_NOFS);
	...
	bytes_left = copy_from_user(lvb_buf, buf, writelen);

net/sunrpc/auth_gss/auth_gss.c:

	buf = kmalloc(mlen, GFP_NOFS);
	...
	if (copy_from_user(buf, src, mlen))

Quoted 12 lines · Alexey Dobriyan

Quoted 11 lines · Li Zefan

I notice there are many places doing copy_from_user() which follows kmalloc():

        dst = kmalloc(len, GFP_KERNEL);
        if (!dst)
                return -ENOMEM;
        if (copy_from_user(dst, src, len)) {
		kfree(dst);
		return -EFAULT
	}

Quoted 7 lines

Quoted 3 lines

kmemdup_from_user() is a wrapper of the above code. With this new function, we don't have to write 'len' twice, which can lead to typos/mistakes. It also produces smaller code.

Name totally sucks, it mixes kernel idiom of allocation with purely userspace function.

I'm not good at English, and I don't know why "kernel memory duplicated from user space" is so bad...

or memdup_user() ?

Quoted 5 lines

Quoted 2 lines

A qucik grep shows 250+ places where kmemdup_from_user() *may* be used. I'll prepare a patchset to do this conversion.

250?

I just found out how many copy_from_user() following km/zalloc(), so not all of them are replace-able.

Quoted 3 lines

Let's not add wrapper for every two lines that happen to be used together.

Why not if we have good reasons? And I don't think we can call this "happen to" if there are 250+ of them?

Quoted 2 lines

Why not if we have good reasons? And I don't think we can call this "happen to" if there are 250+ of them?

Please, read through them. This "250+" number suddenly will become like 20, because wrapper is not good enough.