[PATCH 2/2] strndup_user (v3), convert (keyctl)

[PATCH 2/2] strndup_user (v3), convert (keyctl)

[Davi Arnaut]

Convert security/keys/keyctl.c string duplication to strdup_user()

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

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0c62798..3c49385 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -17,6 +17,7 @@
 #include <linux/keyctl.h>
 #include <linux/fs.h>
 #include <linux/capability.h>
+#include <linux/string.h>
 #include <linux/err.h>
 #include <asm/uaccess.h>
 #include "internal.h"
@@ -38,7 +39,7 @@ asmlinkage long sys_add_key(const char _
 	key_ref_t keyring_ref, key_ref;
 	char type[32], *description;
 	void *payload;
-	long dlen, ret;
+	long ret;
 
 	ret = -EINVAL;
 	if (plen > 32767)
@@ -54,24 +55,11 @@ asmlinkage long sys_add_key(const char _
 	if (type[0] == '.')
 		goto error;
 
-	ret = -EFAULT;
-	dlen = strnlen_user(_description, PAGE_SIZE - 1);
-	if (dlen <= 0)
-		goto error;
-
-	ret = -EINVAL;
-	if (dlen > PAGE_SIZE - 1)
-		goto error;
-
-	ret = -ENOMEM;
-	description = kmalloc(dlen + 1, GFP_KERNEL);
-	if (!description)
+	description = strndup_user(_description, PAGE_SIZE);
+	if (IS_ERR(description)) {
+		ret = PTR_ERR(description);
 		goto error;
-	description[dlen] = '\0';
-
-	ret = -EFAULT;
-	if (copy_from_user(description, _description, dlen) != 0)
-		goto error2;
+	}
 
 	/* pull the payload in if one was supplied */
 	payload = NULL;
@@ -136,7 +124,7 @@ asmlinkage long sys_request_key(const ch
 	struct key *key;
 	key_ref_t dest_ref;
 	char type[32], *description, *callout_info;
-	long dlen, ret;
+	long ret;
 
 	/* pull the type into kernel space */
 	ret = strncpy_from_user(type, _type, sizeof(type) - 1);
@@ -149,46 +137,20 @@ asmlinkage long sys_request_key(const ch
 		goto error;
 
 	/* pull the description into kernel space */
-	ret = -EFAULT;
-	dlen = strnlen_user(_description, PAGE_SIZE - 1);
-	if (dlen <= 0)
-		goto error;
-
-	ret = -EINVAL;
-	if (dlen > PAGE_SIZE - 1)
-		goto error;
-
-	ret = -ENOMEM;
-	description = kmalloc(dlen + 1, GFP_KERNEL);
-	if (!description)
+	description = strndup_user(_description, PAGE_SIZE);
+	if (IS_ERR(description)) {
+		ret = PTR_ERR(description);
 		goto error;
-	description[dlen] = '\0';
-
-	ret = -EFAULT;
-	if (copy_from_user(description, _description, dlen) != 0)
-		goto error2;
+	}
 
 	/* pull the callout info into kernel space */
 	callout_info = NULL;
 	if (_callout_info) {
-		ret = -EFAULT;
-		dlen = strnlen_user(_callout_info, PAGE_SIZE - 1);
-		if (dlen <= 0)
-			goto error2;
-
-		ret = -EINVAL;
-		if (dlen > PAGE_SIZE - 1)
-			goto error2;
-
-		ret = -ENOMEM;
-		callout_info = kmalloc(dlen + 1, GFP_KERNEL);
-		if (!callout_info)
+		callout_info = strndup_user(_callout_info, PAGE_SIZE);
+		if (IS_ERR(callout_info)) {
+			ret = PTR_ERR(callout_info);
 			goto error2;
-		callout_info[dlen] = '\0';
-
-		ret = -EFAULT;
-		if (copy_from_user(callout_info, _callout_info, dlen) != 0)
-			goto error3;
+		}
 	}
 
 	/* get the destination keyring if specified */
@@ -264,36 +226,21 @@ long keyctl_get_keyring_ID(key_serial_t 
 long keyctl_join_session_keyring(const char __user *_name)
 {
 	char *name;
-	long nlen, ret;
+	long ret;
 
 	/* fetch the name from userspace */
 	name = NULL;
 	if (_name) {
-		ret = -EFAULT;
-		nlen = strnlen_user(_name, PAGE_SIZE - 1);
-		if (nlen <= 0)
+		name = strndup_user(_name, PAGE_SIZE);
+		if (IS_ERR(name)) {
+			ret = PTR_ERR(name);
 			goto error;
-
-		ret = -EINVAL;
-		if (nlen > PAGE_SIZE - 1)
-			goto error;
-
-		ret = -ENOMEM;
-		name = kmalloc(nlen + 1, GFP_KERNEL);
-		if (!name)
-			goto error;
-		name[nlen] = '\0';
-
-		ret = -EFAULT;
-		if (copy_from_user(name, _name, nlen) != 0)
-			goto error2;
+		}
 	}
 
 	/* join the session */
 	ret = join_session_keyring(name);
 
- error2:
-	kfree(name);
  error:
 	return ret;
 
@@ -566,7 +513,7 @@ long keyctl_keyring_search(key_serial_t 
 	struct key_type *ktype;
 	key_ref_t keyring_ref, key_ref, dest_ref;
 	char type[32], *description;
-	long dlen, ret;
+	long ret;
 
 	/* pull the type and description into kernel space */
 	ret = strncpy_from_user(type, _type, sizeof(type) - 1);
@@ -574,24 +521,11 @@ long keyctl_keyring_search(key_serial_t 
 		goto error;
 	type[31] = '\0';
 
-	ret = -EFAULT;
-	dlen = strnlen_user(_description, PAGE_SIZE - 1);
-	if (dlen <= 0)
-		goto error;
-
-	ret = -EINVAL;
-	if (dlen > PAGE_SIZE - 1)
+	description = strndup_user(_description, PAGE_SIZE);
+	if (IS_ERR(description)) {
+		ret = PTR_ERR(description);
 		goto error;
-
-	ret = -ENOMEM;
-	description = kmalloc(dlen + 1, GFP_KERNEL);
-	if (!description)
-		goto error;
-	description[dlen] = '\0';
-
-	ret = -EFAULT;
-	if (copy_from_user(description, _description, dlen) != 0)
-		goto error2;
+	}
 
 	/* get the keyring at which to begin the search */
 	keyring_ref = lookup_user_key(NULL, ringid, 0, 0, KEY_SEARCH);

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[David Howells]

Davi Arnaut <davi.arnaut@gmail.com> wrote:

> Convert security/keys/keyctl.c string duplication to strdup_user()

Can you redo this on top of hose patches I submitted yesterday please?

David

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[Davi Arnaut]

On Sat, 18 Feb 2006 16:44:49 +0000
David Howells <dhowells@redhat.com> wrote:

> 
> Davi Arnaut <davi.arnaut@gmail.com> wrote:
> 
> > Convert security/keys/keyctl.c string duplication to strdup_user()
> 
> Can you redo this on top of hose patches I submitted yesterday please?
> 
> David

I think you should just tell Andrew to drop
keys-deal-properly-with-strnlen_user.patch
in favor of mine... :-)

--
Davi Arnaut

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[David Howells]

Davi Arnaut <davi.arnaut@gmail.com> wrote:

> I think you should just tell Andrew to drop
> keys-deal-properly-with-strnlen_user.patch
> in favor of mine... :-)

No... you've taken out all the checks on lengths on NUL-terminated strings.

David

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> > I think you should just tell Andrew to drop
> > keys-deal-properly-with-strnlen_user.patch
> > in favor of mine... :-)
> 
> No... you've taken out all the checks on lengths on NUL-terminated strings.

I take that back... strndup not strdup.

However, the check on the length of the type is wrong with your patch (and in
the unpatched kernel). Can you pull in that bit from my patch?

David

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[Davi Arnaut]

On Mon, 20 Feb 2006 10:38:16 +0000
David Howells <dhowells@redhat.com> wrote:

> David Howells <dhowells@redhat.com> wrote:
> 
> > > I think you should just tell Andrew to drop
> > > keys-deal-properly-with-strnlen_user.patch
> > > in favor of mine... :-)
> > 
> > No... you've taken out all the checks on lengths on NUL-terminated strings.
> 
> I take that back... strndup not strdup.
> 
> However, the check on the length of the type is wrong with your patch (and in
> the unpatched kernel). Can you pull in that bit from my patch?

In keyctl_keyring_search() there wasn't a check for type[0] == '.', but your
mm-patch added one implicitly. Which one is correct ?

> 
> David

Convert security/keys/keyctl.c to use strndup_user() and moves
the type string duplication code to a function.

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

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 0c62798..ed71d86 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -17,10 +17,33 @@
 #include <linux/keyctl.h>
 #include <linux/fs.h>
 #include <linux/capability.h>
+#include <linux/string.h>
 #include <linux/err.h>
 #include <asm/uaccess.h>
 #include "internal.h"
 
+static int key_get_type_from_user(char *type,
+				  const char __user *_type,
+				  unsigned len)
+{
+	int ret;
+
+	ret = strncpy_from_user(type, _type, len);
+
+	if (ret < 0)
+		return -EFAULT;
+
+	if (ret == 0 || ret >= len)
+		return -EINVAL;
+
+	if (type[0] == '.')
+		return -EPERM;
+
+	type[len - 1] = '\0';
+
+	return 0;
+}
+
 /*****************************************************************************/
 /*
  * extract the description of a new key from userspace and either add it as a
@@ -38,40 +61,22 @@ asmlinkage long sys_add_key(const char _
 	key_ref_t keyring_ref, key_ref;
 	char type[32], *description;
 	void *payload;
-	long dlen, ret;
+	long ret;
 
 	ret = -EINVAL;
 	if (plen > 32767)
 		goto error;
 
 	/* draw all the data into kernel space */
-	ret = strncpy_from_user(type, _type, sizeof(type) - 1);
+	ret = key_get_type_from_user(type, _type, sizeof(type));
 	if (ret < 0)
 		goto error;
-	type[31] = '\0';
-
-	ret = -EPERM;
-	if (type[0] == '.')
-		goto error;
-
-	ret = -EFAULT;
-	dlen = strnlen_user(_description, PAGE_SIZE - 1);
-	if (dlen <= 0)
-		goto error;
 
-	ret = -EINVAL;
-	if (dlen > PAGE_SIZE - 1)
-		goto error;
-
-	ret = -ENOMEM;
-	description = kmalloc(dlen + 1, GFP_KERNEL);
-	if (!description)
+	description = strndup_user(_description, PAGE_SIZE);
+	if (IS_ERR(description)) {
+		ret = PTR_ERR(description);
 		goto error;
-	description[dlen] = '\0';
-
-	ret = -EFAULT;
-	if (copy_from_user(description, _description, dlen) != 0)
-		goto error2;
+	}
 
 	/* pull the payload in if one was supplied */
 	payload = NULL;
@@ -136,59 +141,28 @@ asmlinkage long sys_request_key(const ch
 	struct key *key;
 	key_ref_t dest_ref;
 	char type[32], *description, *callout_info;
-	long dlen, ret;
+	long ret;
 
 	/* pull the type into kernel space */
-	ret = strncpy_from_user(type, _type, sizeof(type) - 1);
+	ret = key_get_type_from_user(type, _type, sizeof(type));
 	if (ret < 0)
 		goto error;
-	type[31] = '\0';
-
-	ret = -EPERM;
-	if (type[0] == '.')
-		goto error;
 
 	/* pull the description into kernel space */
-	ret = -EFAULT;
-	dlen = strnlen_user(_description, PAGE_SIZE - 1);
-	if (dlen <= 0)
-		goto error;
-
-	ret = -EINVAL;
-	if (dlen > PAGE_SIZE - 1)
-		goto error;
-
-	ret = -ENOMEM;
-	description = kmalloc(dlen + 1, GFP_KERNEL);
-	if (!description)
+	description = strndup_user(_description, PAGE_SIZE);
+	if (IS_ERR(description)) {
+		ret = PTR_ERR(description);
 		goto error;
-	description[dlen] = '\0';
-
-	ret = -EFAULT;
-	if (copy_from_user(description, _description, dlen) != 0)
-		goto error2;
+	}
 
 	/* pull the callout info into kernel space */
 	callout_info = NULL;
 	if (_callout_info) {
-		ret = -EFAULT;
-		dlen = strnlen_user(_callout_info, PAGE_SIZE - 1);
-		if (dlen <= 0)
-			goto error2;
-
-		ret = -EINVAL;
-		if (dlen > PAGE_SIZE - 1)
-			goto error2;
-
-		ret = -ENOMEM;
-		callout_info = kmalloc(dlen + 1, GFP_KERNEL);
-		if (!callout_info)
+		callout_info = strndup_user(_callout_info, PAGE_SIZE);
+		if (IS_ERR(callout_info)) {
+			ret = PTR_ERR(callout_info);
 			goto error2;
-		callout_info[dlen] = '\0';
-
-		ret = -EFAULT;
-		if (copy_from_user(callout_info, _callout_info, dlen) != 0)
-			goto error3;
+		}
 	}
 
 	/* get the destination keyring if specified */
@@ -264,36 +238,21 @@ long keyctl_get_keyring_ID(key_serial_t 
 long keyctl_join_session_keyring(const char __user *_name)
 {
 	char *name;
-	long nlen, ret;
+	long ret;
 
 	/* fetch the name from userspace */
 	name = NULL;
 	if (_name) {
-		ret = -EFAULT;
-		nlen = strnlen_user(_name, PAGE_SIZE - 1);
-		if (nlen <= 0)
-			goto error;
-
-		ret = -EINVAL;
-		if (nlen > PAGE_SIZE - 1)
+		name = strndup_user(_name, PAGE_SIZE);
+		if (IS_ERR(name)) {
+			ret = PTR_ERR(name);
 			goto error;
-
-		ret = -ENOMEM;
-		name = kmalloc(nlen + 1, GFP_KERNEL);
-		if (!name)
-			goto error;
-		name[nlen] = '\0';
-
-		ret = -EFAULT;
-		if (copy_from_user(name, _name, nlen) != 0)
-			goto error2;
+		}
 	}
 
 	/* join the session */
 	ret = join_session_keyring(name);
 
- error2:
-	kfree(name);
  error:
 	return ret;
 
@@ -566,32 +525,18 @@ long keyctl_keyring_search(key_serial_t 
 	struct key_type *ktype;
 	key_ref_t keyring_ref, key_ref, dest_ref;
 	char type[32], *description;
-	long dlen, ret;
+	long ret;
 
 	/* pull the type and description into kernel space */
-	ret = strncpy_from_user(type, _type, sizeof(type) - 1);
+	ret = key_get_type_from_user(type, _type, sizeof(type));
 	if (ret < 0)
 		goto error;
-	type[31] = '\0';
 
-	ret = -EFAULT;
-	dlen = strnlen_user(_description, PAGE_SIZE - 1);
-	if (dlen <= 0)
+	description = strndup_user(_description, PAGE_SIZE);
+	if (IS_ERR(description)) {
+		ret = PTR_ERR(description);
 		goto error;
-
-	ret = -EINVAL;
-	if (dlen > PAGE_SIZE - 1)
-		goto error;
-
-	ret = -ENOMEM;
-	description = kmalloc(dlen + 1, GFP_KERNEL);
-	if (!description)
-		goto error;
-	description[dlen] = '\0';
-
-	ret = -EFAULT;
-	if (copy_from_user(description, _description, dlen) != 0)
-		goto error2;
+	}
 
 	/* get the keyring at which to begin the search */
 	keyring_ref = lookup_user_key(NULL, ringid, 0, 0, KEY_SEARCH);

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[David Howells]

Davi Arnaut <davi.arnaut@gmail.com> wrote:

> In keyctl_keyring_search() there wasn't a check for type[0] == '.', but your
> mm-patch added one implicitly. Which one is correct ?

Ummm... good point. Key types beginning with a dot are special, and userspace
isn't allowed to create them. I'm not sure whether they should be findable or
not, but I'm happy go for not at the moment (so the patch is correct, not the
original).

There's another minor problem with your patch:

	warthog>grep -r strndup_user *
	warthog1>

I take it that this isn't in Linus's kernel yet... However, I don't want my
patch to be held up too much since there are some awkward holes that need
fixing. I'm definitely in favour of strndup_user() though.

David

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[Davi Arnaut]

On Wed, 01 Mar 2006 14:06:20 +0000
David Howells <dhowells@redhat.com> wrote:

> Davi Arnaut <davi.arnaut@gmail.com> wrote:
> 
> > In keyctl_keyring_search() there wasn't a check for type[0] == '.', but your
> > mm-patch added one implicitly. Which one is correct ?
> 
> Ummm... good point. Key types beginning with a dot are special, and userspace
> isn't allowed to create them. I'm not sure whether they should be findable or
> not, but I'm happy go for not at the moment (so the patch is correct, not the
> original).
> 
> There's another minor problem with your patch:
> 
> 	warthog>grep -r strndup_user *
> 	warthog1>
> 
> I take it that this isn't in Linus's kernel yet... However, I don't want my
> patch to be held up too much since there are some awkward holes that need
> fixing. I'm definitely in favour of strndup_user() though.
> 
> David

Andrew, are you willing to merge for .17 ?

--
Davi Arnaut

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[Andrew Morton]

Davi Arnaut <davi.arnaut@gmail.com> wrote:
>
> On Wed, 01 Mar 2006 14:06:20 +0000
> David Howells <dhowells@redhat.com> wrote:
> 
> > Davi Arnaut <davi.arnaut@gmail.com> wrote:
> > 
> > > In keyctl_keyring_search() there wasn't a check for type[0] == '.', but your
> > > mm-patch added one implicitly. Which one is correct ?
> > 
> > Ummm... good point. Key types beginning with a dot are special, and userspace
> > isn't allowed to create them. I'm not sure whether they should be findable or
> > not, but I'm happy go for not at the moment (so the patch is correct, not the
> > original).
> > 
> > There's another minor problem with your patch:
> > 
> > 	warthog>grep -r strndup_user *
> > 	warthog1>
> > 
> > I take it that this isn't in Linus's kernel yet... However, I don't want my
> > patch to be held up too much since there are some awkward holes that need
> > fixing. I'm definitely in favour of strndup_user() though.
> > 
> > David
> 
> Andrew, are you willing to merge for .17 ?
> 

Which, strndup_user()?

I guess so - would need to see it again.  But David's
keys-deal-properly-with-strnlen_user.patch should go first, since it
kinda-fixes things.

<remembers that macro, shudders>

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[Davi Arnaut]

On Wed, 1 Mar 2006 12:50:53 -0800
Andrew Morton <akpm@osdl.org> wrote:

> Davi Arnaut <davi.arnaut@gmail.com> wrote:
> >
> > On Wed, 01 Mar 2006 14:06:20 +0000
> > David Howells <dhowells@redhat.com> wrote:
> > 
> > > Davi Arnaut <davi.arnaut@gmail.com> wrote:
> > > 
> > > > In keyctl_keyring_search() there wasn't a check for type[0] == '.', but your
> > > > mm-patch added one implicitly. Which one is correct ?
> > > 
> > > Ummm... good point. Key types beginning with a dot are special, and userspace
> > > isn't allowed to create them. I'm not sure whether they should be findable or
> > > not, but I'm happy go for not at the moment (so the patch is correct, not the
> > > original).
> > > 
> > > There's another minor problem with your patch:
> > > 
> > > 	warthog>grep -r strndup_user *
> > > 	warthog1>
> > > 
> > > I take it that this isn't in Linus's kernel yet... However, I don't want my
> > > patch to be held up too much since there are some awkward holes that need
> > > fixing. I'm definitely in favour of strndup_user() though.
> > > 
> > > David
> > 
> > Andrew, are you willing to merge for .17 ?
> > 
> 
> Which, strndup_user()?
> 
> I guess so - would need to see it again.  But David's
> keys-deal-properly-with-strnlen_user.patch should go first, since it
> kinda-fixes things.

Ok, I will resend.

> <remembers that macro, shudders>

I think that's ok with David if we drop it and merge mine. It's much simpler.

--
Davi Arnaut

Re: [PATCH 2/2] strndup_user (v3), convert (keyctl)

[David Howells]

Andrew Morton <akpm@osdl.org> wrote:

> Which, strndup_user()?
> 
> I guess so - would need to see it again.  But David's
> keys-deal-properly-with-strnlen_user.patch should go first, since it
> kinda-fixes things.

I'm happy to let Davi's latest patches displace mine, as they seem to do
everything I want of them.

I'll test them tomorrow (it's way past time to grab a curry and go home).

David