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

Groups > linux.kernel > #1731343 > unrolled thread

[PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

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 2/2] integrity: replace call to integrity_read_file with kernel version Mimi Zohar <zohar@linux.vnet.ibm.com> - 2017-09-13 04:50 +0200
    Re: [PATCH 2/2] integrity: replace call to integrity_read_file with  kernel version James Morris <jmorris@namei.org> - 2017-09-14 22:30 +0200
      Re: [PATCH 2/2] integrity: replace call to integrity_read_file with  kernel version Christoph Hellwig <hch@infradead.org> - 2017-09-14 23:00 +0200
        Re: [PATCH 2/2] integrity: replace call to integrity_read_file with  kernel version James Morris <jmorris@namei.org> - 2017-09-14 23:10 +0200
        Re: [PATCH 2/2] integrity: replace call to integrity_read_file with  kernel version James Morris <jmorris@namei.org> - 2017-09-15 01:30 +0200

#1731343 — [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

From: Christoph Hellwig <hch@lst.de>

The CONFIG_IMA_LOAD_X509 and CONFIG_EVM_LOAD_X509 options permit
loading x509 signed certificates onto the trusted keyrings without
verifying the x509 certificate file's signature.

This patch replaces the call to the integrity_read_file() specific
function with the common kernel_read_file_from_path() function.
To avoid verifying the file signature, this patch defines
READING_X509_CERTFICATE.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

---
Changelog:
- rewrote patch description
- fixed parameters to kernel_read_file_from_path() and key_create_or_update()
- defined READING_X509_CERTIFICATE, a new __kernel_read_file enumeration
- removed constify change
---
 include/linux/fs.h                |  1 +
 security/integrity/digsig.c       | 14 +++++++----
 security/integrity/iint.c         | 49 ---------------------------------------
 security/integrity/ima/ima_main.c |  4 ++++
 security/integrity/integrity.h    |  2 --
 5 files changed, 14 insertions(+), 56 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index d783cc8340de..e522d25d0836 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2751,6 +2751,7 @@ extern int do_pipe_flags(int *, int);
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
 	id(POLICY, security-policy)		\
+	id(X509_CERTIFICATE, x509-certificate)	\
 	id(MAX_ID, )
 
 #define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..6f9e4ce568cd 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,21 +112,25 @@ 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;
+	loff_t size;
 	int rc;
 
 	if (!keyring[id])
 		return -EINVAL;
 
-	rc = integrity_read_file(path, &data);
-	if (rc < 0)
+	rc = kernel_read_file_from_path(path, &data, &size, 0,
+					READING_X509_CERTIFICATE);
+	if (rc < 0) {
+		pr_err("Unable to open file: %s (%d)", path, rc);
 		return rc;
+	}
 
 	key = key_create_or_update(make_key_ref(keyring[id], 1),
 				   "asymmetric",
 				   NULL,
 				   data,
-				   rc,
+				   size,
 				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
 				    KEY_USR_VIEW | KEY_USR_READ),
 				   KEY_ALLOC_NOT_IN_QUOTA);
@@ -139,6 +143,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/ima/ima_main.c b/security/integrity/ima/ima_main.c
index b00186914df8..72bd2b666a31 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -413,6 +413,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
 		return 0;
 
+	/* permit signed certs */
+	if (!file && read_id == READING_X509_CERTIFICATE)
+		return 0;
+
 	if (!file || !buf || size == 0) { /* should never happen */
 		if (ima_appraise & IMA_APPRAISE_ENFORCE)
 			return -EACCES;
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
-- 
2.7.4

[toc] | [next] | [standalone]

#1732559 — Re: [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

On Tue, 12 Sep 2017, Mimi Zohar wrote:

> From: Christoph Hellwig <hch@lst.de>
> 
> The CONFIG_IMA_LOAD_X509 and CONFIG_EVM_LOAD_X509 options permit
> loading x509 signed certificates onto the trusted keyrings without
> verifying the x509 certificate file's signature.
> 
> This patch replaces the call to the integrity_read_file() specific
> function with the common kernel_read_file_from_path() function.
> To avoid verifying the file signature, this patch defines
> READING_X509_CERTFICATE.

So, to be clear, this patch solves the XFS deadlock using a different 
approach (to the now reverted integrity_read approach), which Christoph 
also says is more correct generally.  Correct?

What testing has this had?

Should this go in with the rest of the security changes now or wait until 
either -rc or the next merge window?


-- 
James Morris
<jmorris@namei.org>

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

#1732572 — Re: [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

On Fri, Sep 15, 2017 at 06:21:28AM +1000, James Morris wrote:
> So, to be clear, this patch solves the XFS deadlock using a different 
> approach (to the now reverted integrity_read approach), which Christoph 
> also says is more correct generally.  Correct?

No.  It is in addition to the previous patches - the patches were
correct for the IMA interaction with the I/O path.  It just turns
out that the function was also reused for reading certificates
at initialization time, for which that change was incorrect.

If this series is applied first the integrity_read code is not
used for that path any more.

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

#1732577 — Re: [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

On Thu, 14 Sep 2017, Christoph Hellwig wrote:

> On Fri, Sep 15, 2017 at 06:21:28AM +1000, James Morris wrote:
> > So, to be clear, this patch solves the XFS deadlock using a different 
> > approach (to the now reverted integrity_read approach), which Christoph 
> > also says is more correct generally.  Correct?
> 
> No.  It is in addition to the previous patches - the patches were
> correct for the IMA interaction with the I/O path.  It just turns
> out that the function was also reused for reading certificates
> at initialization time, for which that change was incorrect.
> 
> If this series is applied first the integrity_read code is not
> used for that path any more.

Ok, Mimi, please post a complete patchset for this issue against my -next 
branch.


-- 
James Morris
<jmorris@namei.org>

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

#1732643 — Re: [PATCH 2/2] integrity: replace call to integrity_read_file with kernel version

On Thu, 14 Sep 2017, Christoph Hellwig wrote:

> On Fri, Sep 15, 2017 at 06:21:28AM +1000, James Morris wrote:
> > So, to be clear, this patch solves the XFS deadlock using a different 
> > approach (to the now reverted integrity_read approach), which Christoph 
> > also says is more correct generally.  Correct?
> 
> No.  It is in addition to the previous patches - the patches were
> correct for the IMA interaction with the I/O path.  It just turns
> out that the function was also reused for reading certificates
> at initialization time, for which that change was incorrect.
> 
> If this series is applied first the integrity_read code is not
> used for that path any more.

Ok.  Sorry I hadn't looked at the code in detail at this stage during the 
conference and wanting to just revert back to something that Linus can 
safely pull before the merge window closes.


-- 
James Morris
<jmorris@namei.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web