All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mimi Zohar <zohar@linux.vnet.ibm.com>
To: Christoph Hellwig <hch@infradead.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Cc: James Morris <jmorris@namei.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	Dmitry Kasatkin <dmitry.kasatkin@intel.com>,
	5ac7eace2d00eab5ae0e9fdee63e38aee6001f7c@infradead.org
Subject: Re: [GIT PULL] Security subsystem updates for 4.14
Date: Sun, 10 Sep 2017 10:02:42 -0400	[thread overview]
Message-ID: <1505052162.3224.64.camel@linux.vnet.ibm.com> (raw)
In-Reply-To: <20170910081047.GA19533@infradead.org>

On Sun, 2017-09-10 at 01:10 -0700, Christoph Hellwig wrote:
> On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> > I don't think anybody actually tests linux-next kernels in any big
> > way, and the automated tests that do get run probably don't run with
> > any integrity checking enabled.
> 
> Well, for the atual IMA deadlock issue I asked Mimi to produce automated
> tests and we get started on it.  I was pretty pissed about the
> assumptions IMA made on the fs, whch weren't documented or automatically
> tested - coming from the XFS background where we want all our features
> to run through automated tests that was just not how I'd expect thing
> to work.
> 
> But now as part of that I messed up the other caller of it, so I
> shouldn't complain too loud..
> 
> That being said - I really hink the certificate loading should not
> even go thorught this whole call path, but use our common helper to
> load a file into a buffer.  Something like the patch below, I'm just
> not sure if the last policy argument is what people want or if we'd need
> to add a new policy type for certificates.

We need to differentiate between policies and x509 certificates.  In
the policy case, they need to be signed and appraised, while in the
x509 certificate case, the certificate itself is signed so the file
doesn't need to be signed or verified.

> 
> ---
> From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sun, 10 Sep 2017 09:49:45 +0200
> Subject: security/digisig: read file using kernel_read_file_from_path
> 
> This avoid using the new integrity_read file operation which requires
> i_rwsem already to be held, and avoids a lot of code duplication and
> call the proper LSM hooks.
> 
> [also constifies the path argument to kernel_read_file_from_path,
> as the callers needs it. Should probably be split into a separate patch]
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/exec.c                      |  2 +-
>  include/linux/fs.h             |  2 +-
>  security/integrity/digsig.c    |  8 ++++---
>  security/integrity/iint.c      | 49 ------------------------------------------
>  security/integrity/integrity.h |  2 --
>  5 files changed, 7 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 01a9fb9d8ac3..957a8ce294af 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>  }
>  EXPORT_SYMBOL_GPL(kernel_read_file);
> 
> -int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>  			       loff_t max_size, enum kernel_read_file_id id)
>  {
>  	struct file *file;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2d0e6748e46e..58855daba1eb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
>  extern int kernel_read(struct file *, loff_t, char *, unsigned long);
>  extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
>  			    enum kernel_read_file_id);
> -extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> +extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
>  				      enum kernel_read_file_id);
>  extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
>  				    enum kernel_read_file_id);
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 06554c448dce..8112cdeeee3c 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
>  int __init integrity_load_x509(const unsigned int id, const char *path)
>  {
>  	key_ref_t key;
> -	char *data;
> +	void *data = NULL;
> +	loff_t size;
>  	int rc;
> 
>  	if (!keyring[id])
>  		return -EINVAL;
> 
> -	rc = integrity_read_file(path, &data);
> +	rc = kernel_read_file_from_path(path, data, &size,
> +			0, READING_POLICY);

READING_X509_CERT?

>  	if (rc < 0)
>  		return rc;
> 
> @@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
>  			  key_ref_to_ptr(key)->description, path);
>  		key_ref_put(key);
>  	}
> -	kfree(data);
> +	vfree(data);
>  	return 0;
>  }
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..c84e05866052 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>  }
> 
>  /*
> - * integrity_read_file - read entire file content into the buffer
> - *
> - * This is function opens a file, allocates the buffer of required
> - * size, read entire file content to the buffer and closes the file
> - *
> - * It is used only by init code.
> - *
> - */
> -int __init integrity_read_file(const char *path, char **data)
> -{
> -	struct file *file;
> -	loff_t size;
> -	char *buf;
> -	int rc = -EINVAL;
> -
> -	if (!path || !*path)
> -		return -EINVAL;
> -
> -	file = filp_open(path, O_RDONLY, 0);
> -	if (IS_ERR(file)) {
> -		rc = PTR_ERR(file);
> -		pr_err("Unable to open file: %s (%d)", path, rc);
> -		return rc;
> -	}
> -
> -	size = i_size_read(file_inode(file));
> -	if (size <= 0)
> -		goto out;
> -
> -	buf = kmalloc(size, GFP_KERNEL);
> -	if (!buf) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> -
> -	rc = integrity_kernel_read(file, 0, buf, size);
> -	if (rc == size) {
> -		*data = buf;
> -	} else {
> -		kfree(buf);
> -		if (rc >= 0)
> -			rc = -EIO;
> -	}
> -out:
> -	fput(file);
> -	return rc;
> -}
> -
> -/*
>   * integrity_load_keys - load integrity keys hook
>   *
>   * Hooks is called from init/main.c:kernel_init_freeable()
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..e1bf040fb110 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -120,8 +120,6 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>  int integrity_kernel_read(struct file *file, loff_t offset,
>  			  void *addr, unsigned long count);
> 
> -int __init integrity_read_file(const char *path, char **data);
> -
>  #define INTEGRITY_KEYRING_EVM		0
>  #define INTEGRITY_KEYRING_IMA		1
>  #define INTEGRITY_KEYRING_MODULE	2

WARNING: multiple messages have this Message-ID (diff)
From: zohar@linux.vnet.ibm.com (Mimi Zohar)
To: linux-security-module@vger.kernel.org
Subject: [GIT PULL] Security subsystem updates for 4.14
Date: Sun, 10 Sep 2017 10:02:42 -0400	[thread overview]
Message-ID: <1505052162.3224.64.camel@linux.vnet.ibm.com> (raw)
In-Reply-To: <20170910081047.GA19533@infradead.org>

On Sun, 2017-09-10 at 01:10 -0700, Christoph Hellwig wrote:
> On Fri, Sep 08, 2017 at 10:25:53AM -0700, Linus Torvalds wrote:
> > I don't think anybody actually tests linux-next kernels in any big
> > way, and the automated tests that do get run probably don't run with
> > any integrity checking enabled.
> 
> Well, for the atual IMA deadlock issue I asked Mimi to produce automated
> tests and we get started on it.  I was pretty pissed about the
> assumptions IMA made on the fs, whch weren't documented or automatically
> tested - coming from the XFS background where we want all our features
> to run through automated tests that was just not how I'd expect thing
> to work.
> 
> But now as part of that I messed up the other caller of it, so I
> shouldn't complain too loud..
> 
> That being said - I really hink the certificate loading should not
> even go thorught this whole call path, but use our common helper to
> load a file into a buffer.  Something like the patch below, I'm just
> not sure if the last policy argument is what people want or if we'd need
> to add a new policy type for certificates.

We need to differentiate between policies and x509 certificates. ?In
the policy case, they need to be signed and appraised, while in the
x509 certificate case, the certificate itself is signed so the file
doesn't need to be signed or verified.

> 
> ---
> From 3092ca95dfeff0dbd4e03ae62a51762facb15592 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sun, 10 Sep 2017 09:49:45 +0200
> Subject: security/digisig: read file using kernel_read_file_from_path
> 
> This avoid using the new integrity_read file operation which requires
> i_rwsem already to be held, and avoids a lot of code duplication and
> call the proper LSM hooks.
> 
> [also constifies the path argument to kernel_read_file_from_path,
> as the callers needs it. Should probably be split into a separate patch]
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/exec.c                      |  2 +-
>  include/linux/fs.h             |  2 +-
>  security/integrity/digsig.c    |  8 ++++---
>  security/integrity/iint.c      | 49 ------------------------------------------
>  security/integrity/integrity.h |  2 --
>  5 files changed, 7 insertions(+), 56 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 01a9fb9d8ac3..957a8ce294af 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -974,7 +974,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>  }
>  EXPORT_SYMBOL_GPL(kernel_read_file);
> 
> -int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
>  			       loff_t max_size, enum kernel_read_file_id id)
>  {
>  	struct file *file;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2d0e6748e46e..58855daba1eb 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2791,7 +2791,7 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
>  extern int kernel_read(struct file *, loff_t, char *, unsigned long);
>  extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
>  			    enum kernel_read_file_id);
> -extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> +extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
>  				      enum kernel_read_file_id);
>  extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
>  				    enum kernel_read_file_id);
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 06554c448dce..8112cdeeee3c 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,13 +112,15 @@ int __init integrity_init_keyring(const unsigned int id)
>  int __init integrity_load_x509(const unsigned int id, const char *path)
>  {
>  	key_ref_t key;
> -	char *data;
> +	void *data = NULL;
> +	loff_t size;
>  	int rc;
> 
>  	if (!keyring[id])
>  		return -EINVAL;
> 
> -	rc = integrity_read_file(path, &data);
> +	rc = kernel_read_file_from_path(path, data, &size,
> +			0, READING_POLICY);

READING_X509_CERT?

>  	if (rc < 0)
>  		return rc;
> 
> @@ -139,6 +141,6 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
>  			  key_ref_to_ptr(key)->description, path);
>  		key_ref_put(key);
>  	}
> -	kfree(data);
> +	vfree(data);
>  	return 0;
>  }
> diff --git a/security/integrity/iint.c b/security/integrity/iint.c
> index 6fc888ca468e..c84e05866052 100644
> --- a/security/integrity/iint.c
> +++ b/security/integrity/iint.c
> @@ -200,55 +200,6 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>  }
> 
>  /*
> - * integrity_read_file - read entire file content into the buffer
> - *
> - * This is function opens a file, allocates the buffer of required
> - * size, read entire file content to the buffer and closes the file
> - *
> - * It is used only by init code.
> - *
> - */
> -int __init integrity_read_file(const char *path, char **data)
> -{
> -	struct file *file;
> -	loff_t size;
> -	char *buf;
> -	int rc = -EINVAL;
> -
> -	if (!path || !*path)
> -		return -EINVAL;
> -
> -	file = filp_open(path, O_RDONLY, 0);
> -	if (IS_ERR(file)) {
> -		rc = PTR_ERR(file);
> -		pr_err("Unable to open file: %s (%d)", path, rc);
> -		return rc;
> -	}
> -
> -	size = i_size_read(file_inode(file));
> -	if (size <= 0)
> -		goto out;
> -
> -	buf = kmalloc(size, GFP_KERNEL);
> -	if (!buf) {
> -		rc = -ENOMEM;
> -		goto out;
> -	}
> -
> -	rc = integrity_kernel_read(file, 0, buf, size);
> -	if (rc == size) {
> -		*data = buf;
> -	} else {
> -		kfree(buf);
> -		if (rc >= 0)
> -			rc = -EIO;
> -	}
> -out:
> -	fput(file);
> -	return rc;
> -}
> -
> -/*
>   * integrity_load_keys - load integrity keys hook
>   *
>   * Hooks is called from init/main.c:kernel_init_freeable()
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..e1bf040fb110 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -120,8 +120,6 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode);
>  int integrity_kernel_read(struct file *file, loff_t offset,
>  			  void *addr, unsigned long count);
> 
> -int __init integrity_read_file(const char *path, char **data);
> -
>  #define INTEGRITY_KEYRING_EVM		0
>  #define INTEGRITY_KEYRING_IMA		1
>  #define INTEGRITY_KEYRING_MODULE	2

--
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

  reply	other threads:[~2017-09-10 14:02 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-04 10:29 [GIT PULL] Security subsystem updates for 4.14 James Morris
2017-09-04 10:29 ` James Morris
2017-09-07 18:19 ` Linus Torvalds
2017-09-07 18:19   ` Linus Torvalds
2017-09-08  4:48   ` James Morris
2017-09-08  4:48     ` James Morris
2017-09-08  7:09     ` Christoph Hellwig
2017-09-08  7:09       ` Christoph Hellwig
2017-09-08 17:25       ` Linus Torvalds
2017-09-08 17:25         ` Linus Torvalds
2017-09-08 17:36         ` Paul Moore
2017-09-08 17:36           ` Paul Moore
2017-09-10  4:32           ` James Morris
2017-09-10  4:32             ` James Morris
2017-09-10  4:53             ` James Morris
2017-09-10  4:53               ` James Morris
2017-09-11 22:30             ` Paul Moore
2017-09-11 22:30               ` Paul Moore
2017-09-14 21:09             ` Kees Cook
2017-09-14 21:09               ` Kees Cook
2017-09-14 21:13               ` James Morris
2017-09-14 21:13                 ` James Morris
2017-09-14 21:25                 ` Kees Cook
2017-09-14 21:25                   ` Kees Cook
2017-09-08 19:57         ` James Morris
2017-09-08 19:57           ` James Morris
2017-09-17  7:36           ` Mimi Zohar
2017-09-17  7:36             ` Mimi Zohar
2017-09-10  8:10         ` Christoph Hellwig
2017-09-10  8:10           ` Christoph Hellwig
2017-09-10 14:02           ` Mimi Zohar [this message]
2017-09-10 14:02             ` Mimi Zohar
2017-09-11  6:38             ` Christoph Hellwig
2017-09-11  6:38               ` Christoph Hellwig
2017-09-11 21:34               ` Mimi Zohar
2017-09-11 21:34                 ` Mimi Zohar
2017-09-08 22:38     ` Theodore Ts'o
2017-09-08 22:38       ` Theodore Ts'o
2017-09-10  2:08       ` James Morris
2017-09-10  2:08         ` James Morris
2017-09-10  7:13       ` Mimi Zohar
2017-09-10  7:13         ` Mimi Zohar
2017-09-10 12:17         ` Theodore Ts'o
2017-09-10 12:17           ` Theodore Ts'o
2017-09-10  6:46   ` Mimi Zohar
2017-09-10  6:46     ` Mimi Zohar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1505052162.3224.64.camel@linux.vnet.ibm.com \
    --to=zohar@linux.vnet.ibm.com \
    --cc=5ac7eace2d00eab5ae0e9fdee63e38aee6001f7c@infradead.org \
    --cc=dmitry.kasatkin@intel.com \
    --cc=hch@infradead.org \
    --cc=jmorris@namei.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.