[PATCH] key: Convert big_key payload.data to struct

[PATCH] key: Convert big_key payload.data to struct

[Kees Cook]

There is a lot of needless casting happening in the big_key data payload.
This is harder to trivially verify by static analysis and specifically
the randstruct GCC plugin (which was unhappy about casting a struct
path across two entries of a void * array). This converts the payload to
the actually used structures (one pointer, one embedded struct, and one
size_t).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/key.h     | 11 ++++++++++-
 security/keys/big_key.c | 45 +++++++++++++++++----------------------------
 2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 78e25aabedaf..2390830e3b1a 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -24,6 +24,7 @@
 #include <linux/atomic.h>
 #include <linux/assoc_array.h>
 #include <linux/refcount.h>
+#include <linux/path.h>
 
 #ifdef __KERNEL__
 #include <linux/uidgid.h>
@@ -92,7 +93,15 @@ struct keyring_index_key {
 
 union key_payload {
 	void __rcu		*rcu_data0;
-	void			*data[4];
+	union {
+		void		*data[4];
+		/* Layout of big_key payload words. */
+		struct {
+			u8		*key_data;
+			struct path	key_path;
+			size_t		key_len;
+		};
+	};
 };
 
 /*****************************************************************************/
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 835c1ab30d01..6a0feb964e4a 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -22,16 +22,6 @@
 #include <crypto/skcipher.h>
 
 /*
- * Layout of key payload words.
- */
-enum {
-	big_key_data,
-	big_key_path,
-	big_key_path_2nd_part,
-	big_key_len,
-};
-
-/*
  * Crypto operation with big_key data
  */
 enum big_key_op {
@@ -123,7 +113,7 @@ static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key)
  */
 int big_key_preparse(struct key_preparsed_payload *prep)
 {
-	struct path *path = (struct path *)&prep->payload.data[big_key_path];
+	struct path *path = &prep->payload.key_path;
 	struct file *file;
 	u8 *enckey;
 	u8 *data = NULL;
@@ -138,7 +128,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 	/* Set an arbitrary quota */
 	prep->quotalen = 16;
 
-	prep->payload.data[big_key_len] = (void *)(unsigned long)datalen;
+	prep->payload.key_len = datalen;
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
 		/* Create a shmem file to store the data in.  This will permit the data
@@ -190,7 +180,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		/* Pin the mount and dentry to the key so that we can open it again
 		 * later
 		 */
-		prep->payload.data[big_key_data] = enckey;
+		prep->payload.key_data = enckey;
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
@@ -202,7 +192,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		if (!data)
 			return -ENOMEM;
 
-		prep->payload.data[big_key_data] = data;
+		prep->payload.key_data = data;
 		memcpy(data, prep->data, prep->datalen);
 	}
 	return 0;
@@ -222,11 +212,11 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 void big_key_free_preparse(struct key_preparsed_payload *prep)
 {
 	if (prep->datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&prep->payload.data[big_key_path];
+		struct path *path = &prep->payload.key_path;
 
 		path_put(path);
 	}
-	kfree(prep->payload.data[big_key_data]);
+	kfree(prep->payload.key_data);
 }
 
 /*
@@ -235,12 +225,12 @@ void big_key_free_preparse(struct key_preparsed_payload *prep)
  */
 void big_key_revoke(struct key *key)
 {
-	struct path *path = (struct path *)&key->payload.data[big_key_path];
+	struct path *path = &key->payload.key_path;
 
 	/* clear the quota */
 	key_payload_reserve(key, 0);
 	if (key_is_instantiated(key) &&
-	    (size_t)key->payload.data[big_key_len] > BIG_KEY_FILE_THRESHOLD)
+	    key->payload.key_len > BIG_KEY_FILE_THRESHOLD)
 		vfs_truncate(path, 0);
 }
 
@@ -249,17 +239,17 @@ void big_key_revoke(struct key *key)
  */
 void big_key_destroy(struct key *key)
 {
-	size_t datalen = (size_t)key->payload.data[big_key_len];
+	size_t datalen = key->payload.key_len;
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&key->payload.data[big_key_path];
+		struct path *path = &key->payload.key_path;
 
 		path_put(path);
 		path->mnt = NULL;
 		path->dentry = NULL;
 	}
-	kfree(key->payload.data[big_key_data]);
-	key->payload.data[big_key_data] = NULL;
+	kfree(key->payload.key_data);
+	key->payload.key_data = NULL;
 }
 
 /*
@@ -267,7 +257,7 @@ void big_key_destroy(struct key *key)
  */
 void big_key_describe(const struct key *key, struct seq_file *m)
 {
-	size_t datalen = (size_t)key->payload.data[big_key_len];
+	size_t datalen = key->payload.key_len;
 
 	seq_puts(m, key->description);
 
@@ -283,17 +273,17 @@ void big_key_describe(const struct key *key, struct seq_file *m)
  */
 long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 {
-	size_t datalen = (size_t)key->payload.data[big_key_len];
+	size_t datalen = key->payload.key_len;
 	long ret;
 
 	if (!buffer || buflen < datalen)
 		return datalen;
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&key->payload.data[big_key_path];
+		struct path *path = &key->payload.key_path;
 		struct file *file;
 		u8 *data;
-		u8 *enckey = (u8 *)key->payload.data[big_key_data];
+		u8 *enckey = key->payload.key_data;
 		size_t enclen = ALIGN(datalen, crypto_skcipher_blocksize(big_key_skcipher));
 
 		data = kmalloc(enclen, GFP_KERNEL);
@@ -329,8 +319,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		kfree(data);
 	} else {
 		ret = datalen;
-		if (copy_to_user(buffer, key->payload.data[big_key_data],
-				 datalen) != 0)
+		if (copy_to_user(buffer, key->payload.key_data, datalen) != 0)
 			ret = -EFAULT;
 	}
 
-- 
2.7.4


-- 
Kees Cook
Pixel Security

Re: [PATCH] key: Convert big_key payload.data to struct

[David Howells]

Kees Cook <keescook@chromium.org> wrote:

> There is a lot of needless casting happening in the big_key data payload.
> This is harder to trivially verify by static analysis and specifically
> the randstruct GCC plugin (which was unhappy about casting a struct
> path across two entries of a void * array). This converts the payload to
> the actually used structures (one pointer, one embedded struct, and one
> size_t).

I'd really rather not do this as this moves the definition of an individual
key type into the general structure (I know I've done this for the keyring
type, but that's a special part of the keyring code).  That's the start of the
slippery slope into moving all of them in there.

I'd rather you defined, say:

	struct big_key_payload {
		u8		*key_data;
		struct path	key_path;
		size_t		key_len;
	};

in big_key.c and cast &key->payload to it.

David

Re: [PATCH] key: Convert big_key payload.data to struct

[Eric Biggers]

On Mon, May 08, 2017 at 11:00:56PM +0100, David Howells wrote:
> Kees Cook <keescook@chromium.org> wrote:
> 
> > There is a lot of needless casting happening in the big_key data payload.
> > This is harder to trivially verify by static analysis and specifically
> > the randstruct GCC plugin (which was unhappy about casting a struct
> > path across two entries of a void * array). This converts the payload to
> > the actually used structures (one pointer, one embedded struct, and one
> > size_t).
> 
> I'd really rather not do this as this moves the definition of an individual
> key type into the general structure (I know I've done this for the keyring
> type, but that's a special part of the keyring code).  That's the start of the
> slippery slope into moving all of them in there.
> 
> I'd rather you defined, say:
> 
> 	struct big_key_payload {
> 		u8		*key_data;
> 		struct path	key_path;
> 		size_t		key_len;
> 	};
> 
> in big_key.c and cast &key->payload to it.
> 

That still seems like a hack.  It probably would be easier to kmalloc() this
struct and store a pointer to it in key->payload.data[0], like some of the other
key types do, e.g. "encrypted" and "trusted".  The key data could even be inline
at the end, for non-file backed big_keys.

Eric

Re: [PATCH] key: Convert big_key payload.data to struct

[Kees Cook]

On Mon, May 8, 2017 at 3:00 PM, David Howells <dhowells@redhat.com> wrote:
> Kees Cook <keescook@chromium.org> wrote:
>
>> There is a lot of needless casting happening in the big_key data payload.
>> This is harder to trivially verify by static analysis and specifically
>> the randstruct GCC plugin (which was unhappy about casting a struct
>> path across two entries of a void * array). This converts the payload to
>> the actually used structures (one pointer, one embedded struct, and one
>> size_t).
>
> I'd really rather not do this as this moves the definition of an individual
> key type into the general structure (I know I've done this for the keyring
> type, but that's a special part of the keyring code).  That's the start of the
> slippery slope into moving all of them in there.
>
> I'd rather you defined, say:
>
>         struct big_key_payload {
>                 u8              *key_data;
>                 struct path     key_path;
>                 size_t          key_len;
>         };
>
> in big_key.c and cast &key->payload to it.

This doesn't protect you against changes in struct path size,
though... the existing code (and this proposal) will break if that
ever happens...

What's the problem with defining the types at the top level? That
seems like a nice place to see them all at once.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] key: Convert big_key payload.data to struct

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> It probably would be easier to kmalloc() this struct and store a pointer to
> it in key->payload.data[0]

Yeah, but it's a waste of resources if you don't have to do it.

David

Re: [PATCH] key: Convert big_key payload.data to struct

[David Howells]

Kees Cook <keescook@chromium.org> wrote:

> This doesn't protect you against changes in struct path size,
> though... the existing code (and this proposal) will break if that
> ever happens...

True - in which case you should kmalloc() it as Eric suggests.

> What's the problem with defining the types at the top level? That
> seems like a nice place to see them all at once.

It means adding a bunch of dependencies to linux/key.h and union key_payload.

Have you considered why we don't just put all the definitions for all the
filesystems in linux/fs.h?  By this logic it would seem like a nice place to
see them all at once.

David

Re: [PATCH] key: Convert big_key payload.data to struct

[Kees Cook]

On Tue, May 9, 2017 at 1:11 AM, David Howells <dhowells@redhat.com> wrote:
> Kees Cook <keescook@chromium.org> wrote:
>
>> This doesn't protect you against changes in struct path size,
>> though... the existing code (and this proposal) will break if that
>> ever happens...
>
> True - in which case you should kmalloc() it as Eric suggests.
>
>> What's the problem with defining the types at the top level? That
>> seems like a nice place to see them all at once.
>
> It means adding a bunch of dependencies to linux/key.h and union key_payload.
>
> Have you considered why we don't just put all the definitions for all the
> filesystems in linux/fs.h?  By this logic it would seem like a nice place to
> see them all at once.

I've seen other things that want to share a structure use embedded
structures, etc. I'll see if there is something else to be done, but
just cleaning up the casts alone makes the big_key code so much more
readable. :P

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] key: Convert big_key payload.data to struct

[Eric Biggers]

On Tue, May 09, 2017 at 08:24:18AM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > It probably would be easier to kmalloc() this struct and store a pointer to
> > it in key->payload.data[0]
> 
> Yeah, but it's a waste of resources if you don't have to do it.
> 
> David

Yes, but it seems very much like a micro-optimization, which isn't helpful when
the code contains undefined behavior and is creating problems.  This is the
*big* key type, after all; shouldn't the amount of data in the key normally be
large enough to make a kmalloc() of 24 bytes insignificant?

And besides, I expect that most users don't even use the big_keys feature.  If
we actually want to avoid wasting resources that aren't used, we shouldn't
allocate the crypto_rng and crypto_skcipher until someone tries to create a
big_key.  (Currently they're allocated unconditionally in big_key_init().)

- Eric