| Message ID | 20170508214324.GA124468@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)