Re: [PATCH 0/2] strndup_user, v2

Re: [PATCH 0/2] strndup_user, v2

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 2953 bytes --]

On Wed, 15 Feb 2006 18:22:58 -0300 Davi Arnaut wrote:

> This patch series creates a strndup_user() function in order to avoid duplicated
> and error-prone (userspace modifying the string after the strlen_user()) code.
> 
> v2: Inline strdup_user and fixed a bogus strdup_user usage.
> 
> The diffstat:
> 
>  include/linux/string.h |    7 ++
>  kernel/module.c        |   19 +-------
>  mm/util.c              |   37 +++++++++++++++
>  security/keys/keyctl.c |  116 ++++++++++---------------------------------------
>  4 files changed, 72 insertions(+), 107 deletions(-)
> 
> 
> Signed-off-by: Davi Arnaut <davi.arnaut@gmail.com>
> --
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 369be32..8fbf139 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -18,6 +18,13 @@ extern char * strsep(char **,const char 
>  extern __kernel_size_t strspn(const char *,const char *);
>  extern __kernel_size_t strcspn(const char *,const char *);
>  
> +extern char *strndup_user(const char __user *, long);
> +
> +static inline char *strdup_user(const char __user *s)
> +{
> +	return strndup_user(s, 4096);

PAGE_SIZE ?

> +}
> +
>  /*
>   * Include machine specific inline routines
>   */
> diff --git a/mm/util.c b/mm/util.c
> index 5f4bb59..09c2c3b 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -1,6 +1,8 @@
>  #include <linux/slab.h>
>  #include <linux/string.h>
>  #include <linux/module.h>
> +#include <linux/err.h>
> +#include <asm/uaccess.h>
>  
>  /**
>   * kzalloc - allocate memory. The memory is set to zero.
> @@ -37,3 +39,38 @@ char *kstrdup(const char *s, gfp_t gfp)
>  	return buf;
>  }
>  EXPORT_SYMBOL(kstrdup);
> +
> +/*
> + * strndup_user - duplicate an existing string from user space
> + *
> + * @s: The string to duplicate
> + * @n: Maximum number of bytes to copy, including the trailing NUL.
> + */
> +char *strndup_user(const char __user *s, long n)
> +{
> +	char *p;
> +	long length;
> +
> +	length = strlen_user(s);

This should be strnlen_user(s, n) - no need to look at the whole
potentially huge string if you already have the limit.

> +
> +	if (!length)
> +		return ERR_PTR(-EFAULT);
> +
> +	if (length > n)
> +		length = n;

This silently truncates a too long string, which might not be proper
behavior (in fact, your patch #2 changes the behavior of keyctl
functions, which were rejecting too long strings with -EINVAL - this is
not good).

> +
> +	p = kmalloc(length, GFP_KERNEL);
> +
> +	if (!p)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (strncpy_from_user(p, s, length) < 0) {

This can be plain copy_from_user() - you already have the length, and
even if someone sneaks a NUL byte in the middle, copying bytes past it
will not matter.

> +		kfree(p);
> +		return ERR_PTR(-EFAULT);
> +	}
> +
> +	p[length - 1] = '\0';
> +
> +	return p;
> +}
> +EXPORT_SYMBOL(strndup_user);

[-- Attachment #2: Type: application/pgp-signature, Size: 190 bytes --]

[PATCH 0/2] strndup_user, v2

[Davi Arnaut]

Hi,

This patch series creates a strndup_user() function in order to avoid duplicated
and error-prone (userspace modifying the string after the strlen_user()) code.

v2: Inline strdup_user and fixed a bogus strdup_user usage.

The diffstat:

 include/linux/string.h |    7 ++
 kernel/module.c        |   19 +-------
 mm/util.c              |   37 +++++++++++++++
 security/keys/keyctl.c |  116 ++++++++++---------------------------------------
 4 files changed, 72 insertions(+), 107 deletions(-)


Signed-off-by: Davi Arnaut <davi.arnaut@gmail.com>
--

diff --git a/include/linux/string.h b/include/linux/string.h
index 369be32..8fbf139 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -18,6 +18,13 @@ extern char * strsep(char **,const char 
 extern __kernel_size_t strspn(const char *,const char *);
 extern __kernel_size_t strcspn(const char *,const char *);
 
+extern char *strndup_user(const char __user *, long);
+
+static inline char *strdup_user(const char __user *s)
+{
+	return strndup_user(s, 4096);
+}
+
 /*
  * Include machine specific inline routines
  */
diff --git a/mm/util.c b/mm/util.c
index 5f4bb59..09c2c3b 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -1,6 +1,8 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/module.h>
+#include <linux/err.h>
+#include <asm/uaccess.h>
 
 /**
  * kzalloc - allocate memory. The memory is set to zero.
@@ -37,3 +39,38 @@ char *kstrdup(const char *s, gfp_t gfp)
 	return buf;
 }
 EXPORT_SYMBOL(kstrdup);
+
+/*
+ * strndup_user - duplicate an existing string from user space
+ *
+ * @s: The string to duplicate
+ * @n: Maximum number of bytes to copy, including the trailing NUL.
+ */
+char *strndup_user(const char __user *s, long n)
+{
+	char *p;
+	long length;
+
+	length = strlen_user(s);
+
+	if (!length)
+		return ERR_PTR(-EFAULT);
+
+	if (length > n)
+		length = n;
+
+	p = kmalloc(length, GFP_KERNEL);
+
+	if (!p)
+		return ERR_PTR(-ENOMEM);
+
+	if (strncpy_from_user(p, s, length) < 0) {
+		kfree(p);
+		return ERR_PTR(-EFAULT);
+	}
+
+	p[length - 1] = '\0';
+
+	return p;
+}
+EXPORT_SYMBOL(strndup_user);

Re: [PATCH 0/2] strndup_user, v2

[Alan Cox]

On Mer, 2006-02-15 at 18:22 -0300, Davi Arnaut wrote:
> +static inline char *strdup_user(const char __user *s)
> +{
> +	return strndup_user(s, 4096);
> +}

Still shouldn't exist. Its just a bad idea to give people broken
function they don't yet use.


> +	length = strlen_user(s);

Should use strnlen_user or this function is useless for most cases.

> +
> +	if (!length)
> +		return ERR_PTR(-EFAULT);

Zero isn't an -EFAULT length. Its a null string and valid
> +
> +	if (length > n)
> +		length = n;
> +
> +	p = kmalloc(length, GFP_KERNEL);
> +
> +	if (!p)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (strncpy_from_user(p, s, length) < 0) {
> +		kfree(p);
> +		return ERR_PTR(-EFAULT);
> +	}
> +
> +	p[length - 1] = '\0';

And still broken.

"Hello" -> length = 5   "Hello\0"[4] = 0 "Hell"

Re: [PATCH 0/2] strndup_user, v2

[Davi Arnaut]

On Thu, 16 Feb 2006 01:25:56 +0000
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Mer, 2006-02-15 at 18:22 -0300, Davi Arnaut wrote:
> > +static inline char *strdup_user(const char __user *s)
> > +{
> > +	return strndup_user(s, 4096);
> > +}
> 
> Still shouldn't exist. Its just a bad idea to give people broken
> function they don't yet use.

Ok, I will remove it. But it's a sane default, if someone wants more
than 4096, they should use strndup_user.
 
> > +	length = strlen_user(s);
> 
> Should use strnlen_user or this function is useless for most cases.

Ok.

> > +
> > +	if (!length)
> > +		return ERR_PTR(-EFAULT);
> 
> Zero isn't an -EFAULT length. Its a null string and valid

strlen_user returns _0_ on exception. If you don't belive me,
kernel/module.c or arch/x86_64/lib/usercopy.c are a good starting
point.

> > +
> > +	if (length > n)
> > +		length = n;
> > +
> > +	p = kmalloc(length, GFP_KERNEL);
> > +
> > +	if (!p)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	if (strncpy_from_user(p, s, length) < 0) {
> > +		kfree(p);
> > +		return ERR_PTR(-EFAULT);
> > +	}
> > +
> > +	p[length - 1] = '\0';
> 
> And still broken.
> 
> "Hello" -> length = 5   "Hello\0"[4] = 0 "Hell"
> 

NO! strlen_user("Hello") -> length = 6

strlen_user returns the size of the string INCLUDING the
terminating NUL.

Are we talking in the same language ? 

--
Davi Arnaut

Re: [PATCH 0/2] strndup_user, v2

[Alan Cox]

On Iau, 2006-02-16 at 00:56 -0300, Davi Arnaut wrote:
> strlen_user returns _0_ on exception. If you don't belive me,
> kernel/module.c or arch/x86_64/lib/usercopy.c are a good starting
> point.

My fault, I forgot that someone made strnlen_user weird, the rest looks
correct.

In fact drivers/s390/char/keyboard.c like me appears to have a rather
confused understanding of the return code, but can now make good use of
your strndup_user function.

               len = strnlen_user(u_kbs->kb_string,
                                   sizeof(u_kbs->kb_string) - 1);
                if (!len)
                        return -EFAULT;
                if (len > sizeof(u_kbs->kb_string) - 1)
                        return -EINVAL;
                p = kmalloc(len + 1, GFP_KERNEL);
                if (!p)
                        return -ENOMEM;
                if (copy_from_user(p, u_kbs->kb_string, len)) {
                        kfree(p);
                        return -EFAULT;
                }
                p[len] = 0;


Alan