Convert security/keys/keyctl.c string duplication to strdup_user()
Signed-off-by: Davi Arnaut <[email protected]>
--
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);
Davi Arnaut <[email protected]> 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
On Sat, 18 Feb 2006 16:44:49 +0000
David Howells <[email protected]> wrote:
>
> Davi Arnaut <[email protected]> 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
Davi Arnaut <[email protected]> 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
David Howells <[email protected]> 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
On Mon, 20 Feb 2006 10:38:16 +0000
David Howells <[email protected]> wrote:
> David Howells <[email protected]> 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 <[email protected]>
--
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);
Davi Arnaut <[email protected]> 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
On Wed, 01 Mar 2006 14:06:20 +0000
David Howells <[email protected]> wrote:
> Davi Arnaut <[email protected]> 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
Davi Arnaut <[email protected]> wrote:
>
> On Wed, 01 Mar 2006 14:06:20 +0000
> David Howells <[email protected]> wrote:
>
> > Davi Arnaut <[email protected]> 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>
On Wed, 1 Mar 2006 12:50:53 -0800
Andrew Morton <[email protected]> wrote:
> Davi Arnaut <[email protected]> wrote:
> >
> > On Wed, 01 Mar 2006 14:06:20 +0000
> > David Howells <[email protected]> wrote:
> >
> > > Davi Arnaut <[email protected]> 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
Andrew Morton <[email protected]> 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