Add function strdup_from_user(), which kmallocs a block of memory and
copies a user-space NULL-terminated string into it. NULL is returned if
the string is too large or cannot be accessed.
This function is added to lib/string_helpers.c, so this file is repurposed
to store generic "string helper" functions, and not just a function to
assist in sprintfs.
Signed-off-by: Timur Tabi <[email protected]>
---
include/linux/string_helpers.h | 2 +
lib/string_helpers.c | 43 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 44 insertions(+), 1 deletions(-)
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index a3eb2f6..6231838 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -13,4 +13,6 @@ enum string_size_units {
int string_get_size(u64 size, enum string_size_units units,
char *buf, int len);
+char *strdup_from_user(const char __user *ustr, size_t max);
+
#endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index ab431d4..61323e6 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -1,13 +1,19 @@
/*
- * Helpers for formatting and printing strings
+ * Additional arch-independent string helper functions
*
* Copyright 31 August 2008 James Bottomley
+ * Copyright (C) 2011 Freescale Semiconductor, Inc.
+ *
*/
#include <linux/kernel.h>
#include <linux/math64.h>
#include <linux/module.h>
#include <linux/string_helpers.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <asm/uaccess.h>
+
/**
* string_get_size - get the size in the specified units
* @size: The size to be converted
@@ -66,3 +72,38 @@ int string_get_size(u64 size, const enum string_size_units units,
return 0;
}
EXPORT_SYMBOL(string_get_size);
+
+/**
+ * strdup_from_user - copy a user-space string to a kmalloc'd buffer
+ * @ustr: user-space pointer to null-terminated string
+ * @max: maximum size the string is allowed to be, include NULL terminator
+ *
+ * This function allocates a block of memory (using kmalloc) and copies a
+ * user-space string into that buffer. It returns a pointer to that string,
+ * or an error code.
+ */
+char *strdup_from_user(const char __user *ustr, size_t max)
+{
+ size_t len;
+ char *str;
+
+ len = strnlen_user(ustr, max);
+ if (len >= max)
+ return ERR_PTR(-ENAMETOOLONG);
+
+ /* strnlen_user returns 0 on access error */
+ if (!len)
+ return ERR_PTR(-EFAULT);
+
+ str = kzalloc(len, GFP_KERNEL);
+ if (!str)
+ return ERR_PTR(-ENOMEM);
+
+ if (copy_from_user(str, ustr, len)) {
+ kfree(str);
+ return ERR_PTR(-EFAULT);
+ }
+
+ return str;
+}
+EXPORT_SYMBOL(strdup_from_user);
--
1.7.3.4
NAK
It accesses userspace data twice.
On Fri, Jun 3, 2011 at 7:45 PM, Timur Tabi <[email protected]> wrote:
> Add function strdup_from_user(), which kmallocs a block of memory and
> copies a user-space NULL-terminated string into it. ?NULL is returned if
> the string is too large or cannot be accessed.
>
> This function is added to lib/string_helpers.c, so this file is repurposed
> to store generic "string helper" functions, and not just a function to
> assist in sprintfs.
>
> Signed-off-by: Timur Tabi <[email protected]>
> ---
> ?include/linux/string_helpers.h | ? ?2 +
> ?lib/string_helpers.c ? ? ? ? ? | ? 43 +++++++++++++++++++++++++++++++++++++++-
> ?2 files changed, 44 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> index a3eb2f6..6231838 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -13,4 +13,6 @@ enum string_size_units {
> ?int string_get_size(u64 size, enum string_size_units units,
> ? ? ? ? ? ? ? ? ? ?char *buf, int len);
>
> +char *strdup_from_user(const char __user *ustr, size_t max);
> +
> ?#endif
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index ab431d4..61323e6 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -1,13 +1,19 @@
> ?/*
> - * Helpers for formatting and printing strings
> + * Additional arch-independent string helper functions
> ?*
> ?* Copyright 31 August 2008 James Bottomley
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + *
> ?*/
> ?#include <linux/kernel.h>
> ?#include <linux/math64.h>
> ?#include <linux/module.h>
> ?#include <linux/string_helpers.h>
>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <asm/uaccess.h>
> +
> ?/**
> ?* string_get_size - get the size in the specified units
> ?* @size: ? ? ?The size to be converted
> @@ -66,3 +72,38 @@ int string_get_size(u64 size, const enum string_size_units units,
> ? ? ? ?return 0;
> ?}
> ?EXPORT_SYMBOL(string_get_size);
> +
> +/**
> + * strdup_from_user - copy a user-space string to a kmalloc'd buffer
> + * @ustr: user-space pointer to null-terminated string
> + * @max: maximum size the string is allowed to be, include NULL terminator
> + *
> + * This function allocates a block of memory (using kmalloc) and copies a
> + * user-space string into that buffer. ?It returns a pointer to that string,
> + * or an error code.
> + */
> +char *strdup_from_user(const char __user *ustr, size_t max)
> +{
> + ? ? ? size_t len;
> + ? ? ? char *str;
> +
> + ? ? ? len = strnlen_user(ustr, max);
> + ? ? ? if (len >= max)
> + ? ? ? ? ? ? ? return ERR_PTR(-ENAMETOOLONG);
> +
> + ? ? ? /* strnlen_user returns 0 on access error */
> + ? ? ? if (!len)
> + ? ? ? ? ? ? ? return ERR_PTR(-EFAULT);
> +
> + ? ? ? str = kzalloc(len, GFP_KERNEL);
> + ? ? ? if (!str)
> + ? ? ? ? ? ? ? return ERR_PTR(-ENOMEM);
> +
> + ? ? ? if (copy_from_user(str, ustr, len)) {
> + ? ? ? ? ? ? ? kfree(str);
> + ? ? ? ? ? ? ? return ERR_PTR(-EFAULT);
> + ? ? ? }
> +
> + ? ? ? return str;
> +}
> +EXPORT_SYMBOL(strdup_from_user);
> --
> 1.7.3.4
>
>
> --
> 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/
>
Alexey Dobriyan wrote:
> NAK
> It accesses userspace data twice.
What's wrong with that? How else is it supposed to know how much to allocate
without using strlen first?
Besides, it was Alan's idea to make this a common function. I can easily put it
back in my driver.
--
Timur Tabi
Linux kernel developer at Freescale
On Fri, Jun 3, 2011 at 9:22 PM, Timur Tabi <[email protected]> wrote:
> Alexey Dobriyan wrote:
>> NAK
>> It accesses userspace data twice.
>
> What's wrong with that?
If mm is shared, data can or will change under you.
> How else is it supposed to know how much to allocate
> without using strlen first?
I don't know.
What I know is that your function doesn't guarantee NUL-termination.
> Besides, it was Alan's idea to make this a common function. ?I can easily put it
> back in my driver.
Don't do that.
Alexey Dobriyan wrote:
> If mm is shared, data can or will change under you.
Ah, that's a good point. The only side-effect I can see of that is that it will
copy the string incorrectly, but it won't overwrite memory.
Would it be better if I did this:
str = kzalloc(max, GFP_KERNEL);
if (!str)
return ERR_PTR(-ENOMEM);
if (copy_from_user(str, ustr, max - 1)) {
kfree(str);
return ERR_PTR(-EFAULT);
}
return krealloc(str, strlen(str) + 1, GFP_KERNEL);
Maybe the krealloc() is overkill.
>> > How else is it supposed to know how much to allocate
>> > without using strlen first?
> I don't know.
> What I know is that your function doesn't guarantee NUL-termination.
That's a bug. The copy_from_user() should look like this:
if (copy_from_user(str, ustr, len - 1)) {
--
Timur Tabi
Linux kernel developer at Freescale
On Fri, 3 Jun 2011 21:26:21 +0300
Alexey Dobriyan <[email protected]> wrote:
> On Fri, Jun 3, 2011 at 9:22 PM, Timur Tabi <[email protected]> wrote:
> > Alexey Dobriyan wrote:
> >> NAK
> >> It accesses userspace data twice.
> >
> > What's wrong with that?
>
> If mm is shared, data can or will change under you.
>
> > How else is it supposed to know how much to allocate
> > without using strlen first?
>
> I don't know.
> What I know is that your function doesn't guarantee NUL-termination.
If the only issue is NUL-termination (and I think that's the only issue,
everything else is userspace shooting itself in the foot), just stick
"str[len] = 0" at the end.
-Scott
On Fri, Jun 3, 2011 at 9:34 PM, Timur Tabi <[email protected]> wrote:
> Would it be better if I did this:
The point is data should cross kernelspace/userspace boundary only once.
Alexey Dobriyan wrote:
> The point is data should cross kernelspace/userspace boundary only once.
And my new version does that. I'm just asking if you're okay with it.
Let me repost the whole function:
char *strdup_from_user(const char __user *ustr, size_t max)
{
size_t len;
char *str;
str = kzalloc(max, GFP_KERNEL);
if (!str)
return ERR_PTR(-ENOMEM);
if (copy_from_user(str, ustr, max - 1)) {
kfree(str);
return ERR_PTR(-EFAULT);
}
return krealloc(str, strlen(str) + 1, GFP_KERNEL);
}
--
Timur Tabi
Linux kernel developer at Freescale
On Fri, Jun 3, 2011 at 9:41 PM, Timur Tabi <[email protected]> wrote:
> Alexey Dobriyan wrote:
>> The point is data should cross kernelspace/userspace boundary only once.
>
> And my new version does that. ?I'm just asking if you're okay with it.
It leaks first allocation if second one fails.
Come on.
Show you user in driver, maybe what we're discussing is moot.
On Fri, 3 Jun 2011 21:39:28 +0300
Alexey Dobriyan <[email protected]> wrote:
> On Fri, Jun 3, 2011 at 9:34 PM, Timur Tabi <[email protected]> wrote:
> > Would it be better if I did this:
>
> The point is data should cross kernelspace/userspace boundary only once.
>
Why does it matter, as long as it doesn't hurt the kernel if userspace
plays games (i.e. take care of the NUL termination), and it's not a
performance problem?
-Scott
Alexey Dobriyan wrote:
> It leaks first allocation if second one fails.
>
> Come on.
Ugh, sorry. In my defense, I am suffering from allergies at the moment.
> Show you user in driver, maybe what we're discussing is moot.
The driver and discussion on it can be found here:
http://patchwork.ozlabs.org/patch/98233/
Search for "ioctl_dtprop" to see its usage.
In summary, I'm copying a user-space string into a kmalloc'd buffer so that it's
physically contiguous and I can get the physical address for it. I then call
our hypervisor and give it the physical address. This is why I want a copy of
the string in a buffer allocated via kmalloc.
--
Timur Tabi
Linux kernel developer at Freescale
Scott Wood wrote:
> Why does it matter, as long as it doesn't hurt the kernel if userspace
> plays games (i.e. take care of the NUL termination), and it's not a
> performance problem?
Ok, I don't know why I didn't notice this earlier, but this function already
exists in the kernel. It's called strndup_user().
So I'd like to rescind this patch, please.
Note that strndup_user() has the double-access problem.
--
Timur Tabi
Linux kernel developer at Freescale
On Fri, Jun 3, 2011 at 9:53 PM, Scott Wood <[email protected]> wrote:
> On Fri, 3 Jun 2011 21:39:28 +0300
> Alexey Dobriyan <[email protected]> wrote:
>
>> On Fri, Jun 3, 2011 at 9:34 PM, Timur Tabi <[email protected]> wrote:
>> > Would it be better if I did this:
>>
>> The point is data should cross kernelspace/userspace boundary only once.
>>
>
> Why does it matter, as long as it doesn't hurt the kernel if userspace
> plays games (i.e. take care of the NUL termination), and it's not a
> performance problem?
Because now you're lucky C strings are NUL-terminated.
If this "idiom" applies to some other case like "validate + copy",
we have a bug.
We copy data to kernelspace THEN validate or copy or whatever.
This is obviously correct and safe.
On Fri, 3 Jun 2011 22:12:37 +0300
Alexey Dobriyan <[email protected]> wrote:
> On Fri, Jun 3, 2011 at 9:53 PM, Scott Wood <[email protected]> wrote:
> > On Fri, 3 Jun 2011 21:39:28 +0300
> > Alexey Dobriyan <[email protected]> wrote:
> >
> >> On Fri, Jun 3, 2011 at 9:34 PM, Timur Tabi <[email protected]> wrote:
> >> > Would it be better if I did this:
> >>
> >> The point is data should cross kernelspace/userspace boundary only once.
> >>
> >
> > Why does it matter, as long as it doesn't hurt the kernel if userspace
> > plays games (i.e. take care of the NUL termination), and it's not a
> > performance problem?
>
> Because now you're lucky C strings are NUL-terminated.
> If this "idiom" applies to some other case like "validate + copy",
> we have a bug.
It's not an idiom. It is a simple solution to this particular problem.
> We copy data to kernelspace THEN validate or copy or whatever.
> This is obviously correct and safe.
That doesn't mean that other things are necessarily incorrect or unsafe.
The first access is not validation in this case.
In any case, as it appears there's already a strndup_user() in the kernel,
we'll just use that. You can "fix" it to do a single userspace access, if
you'd like. :-)
-Scott
On Fri, 3 Jun 2011 20:45:10 +0300
Alexey Dobriyan <[email protected]> wrote:
> NAK
> It accesses userspace data twice.
Big deal, processors have caches.
Just needs the zero termination to be reliable.
Alan
On Fri, Jun 03, 2011 at 10:12:37PM +0300, Alexey Dobriyan wrote:
> Because now you're lucky C strings are NUL-terminated.
> If this "idiom" applies to some other case like "validate + copy",
> we have a bug.
>
> We copy data to kernelspace THEN validate or copy or whatever.
> This is obviously correct and safe.
In this case we don't know how _much_ needs to be copied, and that information
comes precisely from NUL-termination. IOW, it's not a matter of luck at all.
"Copy the amount of bytes equal to the limit given to us, then truncate if
needed" is seriously broken in this case. Think what happens if you have
a short string sitting in the middle of a page, with the next page not
mapped at all. And ask to copy up to 4096 bytes. The string itself is
much shorter than that. However, trying to blindly copy those 4096 bytes
will give you -EFAULT. Which is not what we want when copying strings
from userland.
We certainly do not want to *reread* them, but this "find the length, then
copy that much" is just fine.