[PATCH] big_keys: Use struct for internal payload

[PATCH] big_keys: Use struct for internal payload

[Kees Cook]

The randstruct GCC plugin gets upset when it sees struct path (which is
randomized) being assigned from a "void *" (which it cannot type-check).

There's no need for these casts, as the entire internal payload use is
following a normal struct layout. Convert the enum-based void * offset
dereferencing to the new big_key_payload struct. No meaningful machine
code changes result after this change, and source readability is improved.

Drop the randstruct exception now that there is no "confusing" cross-type
assignment.

Cc: David Howells <dhowells@redhat.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-hardening@vger.kernel.org
Cc: keyrings@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/randomize_layout_plugin.c |  2 -
 security/keys/big_key.c                       | 64 ++++++++++---------
 2 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index c2ec81b68505..727512eebb3b 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -50,8 +50,6 @@ static const struct whitelist_entry whitelist[] = {
 	{ "drivers/net/ethernet/sun/niu.c", "page", "address_space" },
 	/* unix_skb_parms via UNIXCB() buffer */
 	{ "net/unix/af_unix.c", "unix_skb_parms", "char" },
-	/* big_key payload.data struct splashing */
-	{ "security/keys/big_key.c", "path", "void *" },
 	{ }
 };
 
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index d17e5f09eeb8..625869939099 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -20,12 +20,13 @@
 /*
  * Layout of key payload words.
  */
-enum {
-	big_key_data,
-	big_key_path,
-	big_key_path_2nd_part,
-	big_key_len,
+struct big_key_payload {
+	u8 *data;
+	struct path path;
+	size_t length;
 };
+#define to_big_key_payload(payload)			\
+	(struct big_key_payload *)((payload).data)
 
 /*
  * If the data is under this limit, there's no point creating a shm file to
@@ -55,7 +56,7 @@ struct key_type key_type_big_key = {
  */
 int big_key_preparse(struct key_preparsed_payload *prep)
 {
-	struct path *path = (struct path *)&prep->payload.data[big_key_path];
+	struct big_key_payload *payload = to_big_key_payload(prep->payload);
 	struct file *file;
 	u8 *buf, *enckey;
 	ssize_t written;
@@ -63,13 +64,15 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 	size_t enclen = datalen + CHACHA20POLY1305_AUTHTAG_SIZE;
 	int ret;
 
+	BUILD_BUG_ON(sizeof(*payload) != sizeof(prep->payload.data));
+
 	if (datalen <= 0 || datalen > 1024 * 1024 || !prep->data)
 		return -EINVAL;
 
 	/* Set an arbitrary quota */
 	prep->quotalen = 16;
 
-	prep->payload.data[big_key_len] = (void *)(unsigned long)datalen;
+	payload->length = datalen;
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
 		/* Create a shmem file to store the data in.  This will permit the data
@@ -117,9 +120,9 @@ 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;
-		*path = file->f_path;
-		path_get(path);
+		payload->data = enckey;
+		payload->path = file->f_path;
+		path_get(&payload->path);
 		fput(file);
 		kvfree_sensitive(buf, enclen);
 	} else {
@@ -129,7 +132,7 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 		if (!data)
 			return -ENOMEM;
 
-		prep->payload.data[big_key_data] = data;
+		payload->data = data;
 		memcpy(data, prep->data, prep->datalen);
 	}
 	return 0;
@@ -148,12 +151,14 @@ int big_key_preparse(struct key_preparsed_payload *prep)
  */
 void big_key_free_preparse(struct key_preparsed_payload *prep)
 {
+	struct big_key_payload *payload = to_big_key_payload(prep->payload);
+
 	if (prep->datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&prep->payload.data[big_key_path];
+		struct path *path = &payload->path;
 
 		path_put(path);
 	}
-	kfree_sensitive(prep->payload.data[big_key_data]);
+	kfree_sensitive(payload->data);
 }
 
 /*
@@ -162,13 +167,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 big_key_payload *payload = to_big_key_payload(key->payload);
 
 	/* clear the quota */
 	key_payload_reserve(key, 0);
-	if (key_is_positive(key) &&
-	    (size_t)key->payload.data[big_key_len] > BIG_KEY_FILE_THRESHOLD)
-		vfs_truncate(path, 0);
+	if (key_is_positive(key) && payload->length > BIG_KEY_FILE_THRESHOLD)
+		vfs_truncate(&payload->path, 0);
 }
 
 /*
@@ -176,17 +180,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];
+	struct big_key_payload *payload = to_big_key_payload(key->payload);
 
-	if (datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&key->payload.data[big_key_path];
+	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
+		struct path *path = &payload->path;
 
 		path_put(path);
 		path->mnt = NULL;
 		path->dentry = NULL;
 	}
-	kfree_sensitive(key->payload.data[big_key_data]);
-	key->payload.data[big_key_data] = NULL;
+	kfree_sensitive(payload->data);
+	payload->data = NULL;
 }
 
 /*
@@ -211,14 +215,14 @@ int big_key_update(struct key *key, struct key_preparsed_payload *prep)
  */
 void big_key_describe(const struct key *key, struct seq_file *m)
 {
-	size_t datalen = (size_t)key->payload.data[big_key_len];
+	struct big_key_payload *payload = to_big_key_payload(key->payload);
 
 	seq_puts(m, key->description);
 
 	if (key_is_positive(key))
 		seq_printf(m, ": %zu [%s]",
-			   datalen,
-			   datalen > BIG_KEY_FILE_THRESHOLD ? "file" : "buff");
+			   payload->length,
+			   payload->length > BIG_KEY_FILE_THRESHOLD ? "file" : "buff");
 }
 
 /*
@@ -227,16 +231,16 @@ void big_key_describe(const struct key *key, struct seq_file *m)
  */
 long big_key_read(const struct key *key, char *buffer, size_t buflen)
 {
-	size_t datalen = (size_t)key->payload.data[big_key_len];
+	struct big_key_payload *payload = to_big_key_payload(key->payload);
+	size_t datalen = payload->length;
 	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 file *file;
-		u8 *buf, *enckey = (u8 *)key->payload.data[big_key_data];
+		u8 *buf, *enckey = payload->data;
 		size_t enclen = datalen + CHACHA20POLY1305_AUTHTAG_SIZE;
 		loff_t pos = 0;
 
@@ -244,7 +248,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
 		if (!buf)
 			return -ENOMEM;
 
-		file = dentry_open(path, O_RDONLY, current_cred());
+		file = dentry_open(&payload->path, O_RDONLY, current_cred());
 		if (IS_ERR(file)) {
 			ret = PTR_ERR(file);
 			goto error;
@@ -274,7 +278,7 @@ long big_key_read(const struct key *key, char *buffer, size_t buflen)
 		kvfree_sensitive(buf, enclen);
 	} else {
 		ret = datalen;
-		memcpy(buffer, key->payload.data[big_key_data], datalen);
+		memcpy(buffer, payload->data, datalen);
 	}
 
 	return ret;
-- 
2.32.0

Re: [PATCH] big_keys: Use struct for internal payload

[Eric Biggers]

On Sun, May 08, 2022 at 10:57:31AM -0700, Kees Cook wrote:
> The randstruct GCC plugin gets upset when it sees struct path (which is
> randomized) being assigned from a "void *" (which it cannot type-check).
> 
> There's no need for these casts, as the entire internal payload use is
> following a normal struct layout. Convert the enum-based void * offset
> dereferencing to the new big_key_payload struct. No meaningful machine
> code changes result after this change, and source readability is improved.
> 
> Drop the randstruct exception now that there is no "confusing" cross-type
> assignment.
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-hardening@vger.kernel.org
> Cc: keyrings@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  scripts/gcc-plugins/randomize_layout_plugin.c |  2 -
>  security/keys/big_key.c                       | 64 ++++++++++---------
>  2 files changed, 34 insertions(+), 32 deletions(-)

This looks fine to me, although the way that an array of void pointers is cast
to/from another struct is still weird.  I'd prefer if the payload was just
changed into a separate allocation.

A couple nits below if you stay with your proposed solution:

>  void big_key_free_preparse(struct key_preparsed_payload *prep)
>  {
> +	struct big_key_payload *payload = to_big_key_payload(prep->payload);
> +
>  	if (prep->datalen > BIG_KEY_FILE_THRESHOLD) {
> -		struct path *path = (struct path *)&prep->payload.data[big_key_path];
> +		struct path *path = &payload->path;
>  
>  		path_put(path);
>  	}

This could just do:

	if (prep->datalen > BIG_KEY_FILE_THRESHOLD)
		path_put(&payload->path);

>  void big_key_destroy(struct key *key)
>  {
> -	size_t datalen = (size_t)key->payload.data[big_key_len];
> +	struct big_key_payload *payload = to_big_key_payload(key->payload);
>  
> -	if (datalen > BIG_KEY_FILE_THRESHOLD) {
> -		struct path *path = (struct path *)&key->payload.data[big_key_path];
> +	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
> +		struct path *path = &payload->path;
>  
>  		path_put(path);
>  		path->mnt = NULL;
>  		path->dentry = NULL;
>  	}

And similarly:

	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
		path_put(&payload->path);
		payload->path.mnt = NULL;
		payload->path.dentry = NULL;
	}

- Eric

Re: [PATCH] big_keys: Use struct for internal payload

[Kees Cook]

On Mon, May 09, 2022 at 04:13:48PM -0700, Eric Biggers wrote:
> On Sun, May 08, 2022 at 10:57:31AM -0700, Kees Cook wrote:
> > The randstruct GCC plugin gets upset when it sees struct path (which is
> > randomized) being assigned from a "void *" (which it cannot type-check).
> > 
> > There's no need for these casts, as the entire internal payload use is
> > following a normal struct layout. Convert the enum-based void * offset
> > dereferencing to the new big_key_payload struct. No meaningful machine
> > code changes result after this change, and source readability is improved.
> > 
> > Drop the randstruct exception now that there is no "confusing" cross-type
> > assignment.
> > 
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-hardening@vger.kernel.org
> > Cc: keyrings@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  scripts/gcc-plugins/randomize_layout_plugin.c |  2 -
> >  security/keys/big_key.c                       | 64 ++++++++++---------
> >  2 files changed, 34 insertions(+), 32 deletions(-)
> 
> This looks fine to me, although the way that an array of void pointers is cast
> to/from another struct is still weird.  I'd prefer if the payload was just
> changed into a separate allocation.

Yeah, though I realized after sending this patch that I'd done it
before[1] back with the rest of the randstruct GCC plugin enabling,
and it seems David was against the separate allocation, which, given the
space available, isn't unreasonable right up until struct path doesn't fit
anymore, but that's why I've added the BUILD_BUG_ON() to check sizes. :)
And this version ended up quite close to what hwh suggested[2] in 2017.

> A couple nits below if you stay with your proposed solution:
> 
> >  void big_key_free_preparse(struct key_preparsed_payload *prep)
> >  {
> > +	struct big_key_payload *payload = to_big_key_payload(prep->payload);
> > +
> >  	if (prep->datalen > BIG_KEY_FILE_THRESHOLD) {
> > -		struct path *path = (struct path *)&prep->payload.data[big_key_path];
> > +		struct path *path = &payload->path;
> >  
> >  		path_put(path);
> >  	}
> 
> This could just do:
> 
> 	if (prep->datalen > BIG_KEY_FILE_THRESHOLD)
> 		path_put(&payload->path);

Sure, I can avoid the extra variable.

> 
> >  void big_key_destroy(struct key *key)
> >  {
> > -	size_t datalen = (size_t)key->payload.data[big_key_len];
> > +	struct big_key_payload *payload = to_big_key_payload(key->payload);
> >  
> > -	if (datalen > BIG_KEY_FILE_THRESHOLD) {
> > -		struct path *path = (struct path *)&key->payload.data[big_key_path];
> > +	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
> > +		struct path *path = &payload->path;
> >  
> >  		path_put(path);
> >  		path->mnt = NULL;
> >  		path->dentry = NULL;
> >  	}
> 
> And similarly:
> 
> 	if (payload->length > BIG_KEY_FILE_THRESHOLD) {
> 		path_put(&payload->path);
> 		payload->path.mnt = NULL;
> 		payload->path.dentry = NULL;
> 	}

I will respin.

Thanks!

-Kees

[1] https://lore.kernel.org/lkml/20170508214324.GA124468@beast/
[2] https://lore.kernel.org/lkml/20170528081249.GD22193@infradead.org/

-- 
Kees Cook