Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1651606 > unrolled thread

[PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading

Started byKees Cook <keescook@chromium.org>
First post2017-05-27 03:20 +0200
Last post2017-05-28 19:00 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading Kees Cook <keescook@chromium.org> - 2017-05-27 03:20 +0200
    Re: [PATCH v2 07/20] randstruct: Whitelist big_key path struct  overloading Christoph Hellwig <hch@infradead.org> - 2017-05-28 10:20 +0200
      Re: [PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading Kees Cook <keescook@chromium.org> - 2017-05-28 19:00 +0200

#1651606 — [PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading

FromKees Cook <keescook@chromium.org>
Date2017-05-27 03:20 +0200
Subject[PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading
Message-ID<tLyyB-4R7-11@gated-at.bofh.it>
The big_key payload structure intentionally stores a struct path in
two void pointers to avoid header soup. Whitelist this case:

security/keys/big_key.c: In function ‘big_key_read’:
security/keys/big_key.c:293:16: note: found mismatched rhs struct pointer types: ‘struct path’ and ‘void *’

   struct path *path = (struct path *)&key->payload.data[big_key_path];
                ^~~~

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 scripts/gcc-plugins/randomize_layout_plugin.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index bf110915a5aa..5f4e59a78eeb 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -45,6 +45,8 @@ struct whitelist_entry {
 static const struct whitelist_entry whitelist[] = {
 	/* 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 *" },
 	/* walk struct security_hook_heads as an array of struct list_head */
 	{ "security/security.c", "list_head", "security_hook_heads" },
 	{ }
-- 
2.7.4

[toc] | [next] | [standalone]

#1652057 — Re: [PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading

FromChristoph Hellwig <hch@infradead.org>
Date2017-05-28 10:20 +0200
SubjectRe: [PATCH v2 07/20] randstruct: Whitelist big_key path struct overloading
Message-ID<tM1AB-7P8-7@gated-at.bofh.it>
In reply to#1651606
What about the untested patch below to just fix the issue?

---
From e9eb519c854d2f3d16a4def492577a883246e290 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Sun, 28 May 2017 11:03:34 +0300
Subject: security/keys: don't cast union key_payload

Instead store the individual pointers in struct path.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 security/keys/big_key.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 835c1ab30d01..06f2cd07dbd7 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -26,8 +26,8 @@
  */
 enum {
 	big_key_data,
-	big_key_path,
-	big_key_path_2nd_part,
+	big_key_path_mnt,
+	big_key_path_dentry,
 	big_key_len,
 };
 
@@ -118,12 +118,16 @@ static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key)
 	return ret;
 }
 
+#define PATH_FROM_PAYLOAD(p) {				\
+	.mnt	= (p)->data[big_key_path_mnt],		\
+	.dentry	= (p)->data[big_key_path_dentry],	\
+}
+
 /*
  * Preparse a big key
  */
 int big_key_preparse(struct key_preparsed_payload *prep)
 {
-	struct path *path = (struct path *)&prep->payload.data[big_key_path];
 	struct file *file;
 	u8 *enckey;
 	u8 *data = NULL;
@@ -190,9 +194,10 @@ 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
 		 */
+		path_get(&file->f_path);
 		prep->payload.data[big_key_data] = enckey;
-		*path = file->f_path;
-		path_get(path);
+		prep->payload.data[big_key_path_mnt] = file->f_path.mnt;
+		prep->payload.data[big_key_path_dentry] = file->f_path.dentry;
 		fput(file);
 		kfree(data);
 	} else {
@@ -222,9 +227,9 @@ 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 = PATH_FROM_PAYLOAD(&prep->payload);
 
-		path_put(path);
+		path_put(&path);
 	}
 	kfree(prep->payload.data[big_key_data]);
 }
@@ -235,13 +240,13 @@ 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 = PATH_FROM_PAYLOAD(&key->payload);
 
 	/* 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)
-		vfs_truncate(path, 0);
+		vfs_truncate(&path, 0);
 }
 
 /*
@@ -252,11 +257,11 @@ void big_key_destroy(struct key *key)
 	size_t datalen = (size_t)key->payload.data[big_key_len];
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&key->payload.data[big_key_path];
+		struct path path = PATH_FROM_PAYLOAD(&key->payload);
 
-		path_put(path);
-		path->mnt = NULL;
-		path->dentry = NULL;
+		path_put(&path);
+		key->payload.data[big_key_path_mnt] = NULL;
+		key->payload.data[big_key_path_dentry] = NULL;
 	}
 	kfree(key->payload.data[big_key_data]);
 	key->payload.data[big_key_data] = NULL;
@@ -290,7 +295,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		return datalen;
 
 	if (datalen > BIG_KEY_FILE_THRESHOLD) {
-		struct path *path = (struct path *)&key->payload.data[big_key_path];
+		struct path path = PATH_FROM_PAYLOAD(&key->payload);
 		struct file *file;
 		u8 *data;
 		u8 *enckey = (u8 *)key->payload.data[big_key_data];
@@ -300,7 +305,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		if (!data)
 			return -ENOMEM;
 
-		file = dentry_open(path, O_RDONLY, current_cred());
+		file = dentry_open(&path, O_RDONLY, current_cred());
 		if (IS_ERR(file)) {
 			ret = PTR_ERR(file);
 			goto error;
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1652176

FromKees Cook <keescook@chromium.org>
Date2017-05-28 19:00 +0200
Message-ID<tM9HQ-4wx-5@gated-at.bofh.it>
In reply to#1652057
On Sun, May 28, 2017 at 1:12 AM, Christoph Hellwig <hch@infradead.org> wrote:
> What about the untested patch below to just fix the issue?
>
> ---
> From e9eb519c854d2f3d16a4def492577a883246e290 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sun, 28 May 2017 11:03:34 +0300
> Subject: security/keys: don't cast union key_payload
>
> Instead store the individual pointers in struct path.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yeah, this is less invasive than what I'd proposed to David to fix it
earlier. David, does this look okay to you?

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  security/keys/big_key.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/security/keys/big_key.c b/security/keys/big_key.c
> index 835c1ab30d01..06f2cd07dbd7 100644
> --- a/security/keys/big_key.c
> +++ b/security/keys/big_key.c
> @@ -26,8 +26,8 @@
>   */
>  enum {
>         big_key_data,
> -       big_key_path,
> -       big_key_path_2nd_part,
> +       big_key_path_mnt,
> +       big_key_path_dentry,
>         big_key_len,
>  };
>
> @@ -118,12 +118,16 @@ static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key)
>         return ret;
>  }
>
> +#define PATH_FROM_PAYLOAD(p) {                         \
> +       .mnt    = (p)->data[big_key_path_mnt],          \
> +       .dentry = (p)->data[big_key_path_dentry],       \
> +}
> +
>  /*
>   * Preparse a big key
>   */
>  int big_key_preparse(struct key_preparsed_payload *prep)
>  {
> -       struct path *path = (struct path *)&prep->payload.data[big_key_path];
>         struct file *file;
>         u8 *enckey;
>         u8 *data = NULL;
> @@ -190,9 +194,10 @@ 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
>                  */
> +               path_get(&file->f_path);
>                 prep->payload.data[big_key_data] = enckey;
> -               *path = file->f_path;
> -               path_get(path);
> +               prep->payload.data[big_key_path_mnt] = file->f_path.mnt;
> +               prep->payload.data[big_key_path_dentry] = file->f_path.dentry;
>                 fput(file);
>                 kfree(data);
>         } else {
> @@ -222,9 +227,9 @@ 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 = PATH_FROM_PAYLOAD(&prep->payload);
>
> -               path_put(path);
> +               path_put(&path);
>         }
>         kfree(prep->payload.data[big_key_data]);
>  }
> @@ -235,13 +240,13 @@ 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 = PATH_FROM_PAYLOAD(&key->payload);
>
>         /* 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)
> -               vfs_truncate(path, 0);
> +               vfs_truncate(&path, 0);
>  }
>
>  /*
> @@ -252,11 +257,11 @@ void big_key_destroy(struct key *key)
>         size_t datalen = (size_t)key->payload.data[big_key_len];
>
>         if (datalen > BIG_KEY_FILE_THRESHOLD) {
> -               struct path *path = (struct path *)&key->payload.data[big_key_path];
> +               struct path path = PATH_FROM_PAYLOAD(&key->payload);
>
> -               path_put(path);
> -               path->mnt = NULL;
> -               path->dentry = NULL;
> +               path_put(&path);
> +               key->payload.data[big_key_path_mnt] = NULL;
> +               key->payload.data[big_key_path_dentry] = NULL;
>         }
>         kfree(key->payload.data[big_key_data]);
>         key->payload.data[big_key_data] = NULL;
> @@ -290,7 +295,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
>                 return datalen;
>
>         if (datalen > BIG_KEY_FILE_THRESHOLD) {
> -               struct path *path = (struct path *)&key->payload.data[big_key_path];
> +               struct path path = PATH_FROM_PAYLOAD(&key->payload);
>                 struct file *file;
>                 u8 *data;
>                 u8 *enckey = (u8 *)key->payload.data[big_key_data];
> @@ -300,7 +305,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
>                 if (!data)
>                         return -ENOMEM;
>
> -               file = dentry_open(path, O_RDONLY, current_cred());
> +               file = dentry_open(&path, O_RDONLY, current_cred());
>                 if (IS_ERR(file)) {
>                         ret = PTR_ERR(file);
>                         goto error;
> --
> 2.11.0
>



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web