key: Convert big_key payload.data to struct

PATCH In review Kees Cook 3 participants 8 messages May 8, 2017 – May 9, 2017

Subsystems (2)

Security (LSM) Keys/keyrings

lore

Share & tools

lore

Kees Cook

author

2017-05-08 21:43 UTC #0 lore raw

PATCH 1/1 key: Convert big_key payload.data to struct

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 <[email protected]>

---
 include/linux/key.h     | 11 ++++++++++-
 security/keys/big_key.c | 45 +++++++++++++++++----------------------------
 2 files changed, 27 insertions(+), 29 deletions(-)

include/linux/key.h +10 -1

index 78e25aabedaf..2390830e3b1a 100644
@@ -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;
+		};
+	};
 };
 
 /*****************************************************************************/

security/keys/big_key.c +17 ~~-28~~

133 lines not on this page · raw message

Signature · 10 lines

2.7.4


-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells

maintainer

2017-05-08 22:00 UTC #1 lore raw

Quoted 6 lines · 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).

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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Biggers

2017-05-08 22:19 UTC #2 lore raw

Quoted 16 lines · David Howells

Quoted 6 lines · 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).
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:

Quoted 8 lines

	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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Howells

maintainer

2017-05-09 07:24 UTC #3 lore raw

Quoted 2 lines · Eric Biggers

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.

Eric Biggers

2017-05-09 21:45 UTC #4 lore raw

Quoted 8 lines · David Howells

Quoted 2 lines · Eric Biggers
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 at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kees Cook

author

2017-05-08 22:26 UTC #5 lore raw

In reply to David Howells

Quoted 16 lines · David Howells

Quoted 6 lines · 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).
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:

Quoted 7 lines

        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

Signature · 6 lines

Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells

maintainer

2017-05-09 08:11 UTC #6 lore raw

Quoted 3 lines · Kees Cook

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.

Quoted 2 lines

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.

Kees Cook

author

2017-05-09 16:12 UTC #7 lore raw

Quoted 8 lines · David Howells

Quoted 3 lines · Kees Cook
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.

Quoted 8 lines

Quoted 2 lines
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

Signature · 6 lines

Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html