[PATCH 0/4] Fix misused kernel_read_file() enums

[PATCH 0/4] Fix misused kernel_read_file() enums

[Kees Cook]

Hi,

In looking for closely at the additions that got made to the
kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
*kinds* of files for the LSM to reason about. They are a "how" and
"where", respectively. Remove these improper aliases and refactor the
code to adapt to the changes.

Additionally adds in missing calls to security_kernel_post_read_file()
in the platform firmware fallback path (to match the sysfs firmware
fallback path) and in module loading. I considered entirely removing
security_kernel_post_read_file() hook since it is technically unused,
but IMA probably wants to be able to measure EFI-stored firmware images,
so I wired it up and matched it for modules, in case anyone wants to
move the module signature checks out of the module core and into an LSM
to avoid the current layering violations.

This touches several trees, and I suspect it would be best to go through
James's LSM tree.

Thanks!

-Kees

Kees Cook (4):
  firmware_loader: EFI firmware loader must handle pre-allocated buffer
  fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
  fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
  module: Add hook for security_kernel_post_read_file()

 drivers/base/firmware_loader/fallback_platform.c | 12 ++++++++++--
 drivers/base/firmware_loader/main.c              |  5 ++---
 fs/exec.c                                        |  7 ++++---
 include/linux/fs.h                               |  3 +--
 include/linux/lsm_hooks.h                        |  6 +++++-
 kernel/module.c                                  |  7 ++++++-
 security/integrity/ima/ima_main.c                |  6 ++----
 7 files changed, 30 insertions(+), 16 deletions(-)

-- 
2.25.1

[PATCH 1/4] firmware_loader: EFI firmware loader must handle pre-allocated buffer

[Kees Cook]

The EFI platform firmware fallback would clobber any pre-allocated
buffers. Instead, correctly refuse to reallocate when too small (as
already done in the sysfs fallback), or perform allocation normally
when needed.

Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm ware_request_platform()")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/base/firmware_loader/fallback_platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index cdd2c9a9f38a..685edb7dd05a 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
 	if (rc)
 		return rc; /* rc == -ENOENT when the fw was not found */
 
-	fw_priv->data = vmalloc(size);
+	if (fw_priv->data && size > fw_priv->allocated_size)
+		return -ENOMEM;
+	if (!fw_priv->data)
+		fw_priv->data = vmalloc(size);
 	if (!fw_priv->data)
 		return -ENOMEM;
 
-- 
2.25.1

[PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Kees Cook]

FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.

Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/base/firmware_loader/main.c | 5 ++---
 fs/exec.c                           | 7 ++++---
 include/linux/fs.h                  | 2 +-
 security/integrity/ima/ima_main.c   | 6 ++----
 4 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 	int i, len;
 	int rc = -ENOENT;
 	char *path;
-	enum kernel_read_file_id id = READING_FIRMWARE;
 	size_t msize = INT_MAX;
 	void *buffer = NULL;
 
 	/* Already populated data member means we're loading into a buffer */
 	if (!decompress && fw_priv->data) {
 		buffer = fw_priv->data;
-		id = READING_FIRMWARE_PREALLOC_BUFFER;
 		msize = fw_priv->allocated_size;
 	}
 
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 
 		/* load firmware files from the mount namespace of init */
 		rc = kernel_read_file_from_path_initns(path, &buffer,
-						       &size, msize, id);
+						       &size, msize,
+						       READING_FIRMWARE);
 		if (rc) {
 			if (rc != -ENOENT)
 				dev_warn(device, "loading %s failed with error %d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 {
 	loff_t i_size, pos;
 	ssize_t bytes = 0;
+	void *allocated = NULL;
 	int ret;
 
 	if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 		goto out;
 	}
 
-	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
-		*buf = vmalloc(i_size);
+	if (!*buf)
+		*buf = allocated = vmalloc(i_size);
 	if (!*buf) {
 		ret = -ENOMEM;
 		goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 
 out_free:
 	if (ret < 0) {
-		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+		if (allocated) {
 			vfree(*buf);
 			*buf = NULL;
 		}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
 #endif
 extern int do_pipe_flags(int *, int);
 
+/* This is a list of *what* is being read, not *how*. */
 #define __kernel_read_file_id(id) \
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
-	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
 	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..f80ee4ce4669 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
 int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 {
 	/*
-	 * READING_FIRMWARE_PREALLOC_BUFFER
-	 *
 	 * Do devices using pre-allocated memory run the risk of the
 	 * firmware being accessible to the device prior to the completion
 	 * of IMA's signature verification any more than when using two
-	 * buffers?
+	 * buffers? It may be desirable to include the buffer address
+	 * in this API and walk all the dma_map_single() mappings to check.
 	 */
 	return 0;
 }
 
 const int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
-	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
 	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
 	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
-- 
2.25.1

[PATCH 3/4] fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums

[Kees Cook]

The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening. Add call to
security_kernel_post_read_file() so the contents can be measured/verified,
just as the firmware sysfs fallback does. This would allow for IMA (or
other LSMs) to validate known-good EFI firmware images.

Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firmware_request_platform()")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/base/firmware_loader/fallback_platform.c | 7 ++++++-
 include/linux/fs.h                               | 3 +--
 include/linux/lsm_hooks.h                        | 6 +++++-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..76e0c4a7835f 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
 	if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
 		return -ENOENT;
 
-	rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+	rc = security_kernel_load_data(LOADING_FIRMWARE);
 	if (rc)
 		return rc;
 
@@ -25,6 +25,11 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
 	if (rc)
 		return rc; /* rc == -ENOENT when the fw was not found */
 
+	rc = security_kernel_post_read_file(NULL, (char *)data, size,
+					    READING_FIRMWARE);
+	if (rc)
+		return rc;
+
 	if (fw_priv->data && size > fw_priv->allocated_size)
 		return -ENOMEM;
 	if (!fw_priv->data)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
 #endif
 extern int do_pipe_flags(int *, int);
 
-/* This is a list of *what* is being read, not *how*. */
+/* This is a list of *what* is being read, not *how* nor *where*. */
 #define __kernel_read_file_id(id) \
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
-	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 95b7c1d32062..7cfc3166a1e2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -633,15 +633,19 @@
  *	@kmod_name name of the module requested by the kernel
  *	Return 0 if successful.
  * @kernel_load_data:
- *	Load data provided by userspace.
+ *	Load data provided by a non-file source (usually userspace buffer).
  *	@id kernel load data identifier
  *	Return 0 if permission is granted.
+ *	This may be paired with a kernel_post_read_file() with a NULL
+ *	@file, but contains the actual data loaded.
  * @kernel_read_file:
  *	Read a file specified by userspace.
  *	@file contains the file structure pointing to the file being read
  *	by the kernel.
  *	@id kernel read file identifier
  *	Return 0 if permission is granted.
+ *	This must be paired with a kernel_post_read_file(), which contains
+ *	the actual data read from @file.
  * @kernel_post_read_file:
  *	Read a file specified by userspace.
  *	@file contains the file structure pointing to the file being read
-- 
2.25.1

[PATCH 4/4] module: Add hook for security_kernel_post_read_file()

[Kees Cook]

Calls to security_kernel_load_data() should be paired with a call to
security_kernel_post_read_file() with a NULL file argument. Add the
missing call so the module contents are visible to the LSMs interested
in measuring the module content. (This also paves the way for moving
module signature checking out of the module core and into an LSM.)

Cc: Jessica Yu <jeyu@kernel.org>
Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/module.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..af9679f8e5c6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
 		return -EFAULT;
 	}
 
-	return 0;
+	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
+					     info->len, READING_MODULE);
+	if (err)
+		vfree(info->hdr);
+
+	return err;
 }
 
 static void free_copy(struct load_info *info)
-- 
2.25.1

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Greg Kroah-Hartman]

On Tue, Jul 07, 2020 at 01:19:22AM -0700, Kees Cook wrote:
> Hi,
> 
> In looking for closely at the additions that got made to the
> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> *kinds* of files for the LSM to reason about. They are a "how" and
> "where", respectively. Remove these improper aliases and refactor the
> code to adapt to the changes.
> 
> Additionally adds in missing calls to security_kernel_post_read_file()
> in the platform firmware fallback path (to match the sysfs firmware
> fallback path) and in module loading. I considered entirely removing
> security_kernel_post_read_file() hook since it is technically unused,
> but IMA probably wants to be able to measure EFI-stored firmware images,
> so I wired it up and matched it for modules, in case anyone wants to
> move the module signature checks out of the module core and into an LSM
> to avoid the current layering violations.
> 
> This touches several trees, and I suspect it would be best to go through
> James's LSM tree.
> 
> Thanks!
> 
> -Kees
> 
> Kees Cook (4):
>   firmware_loader: EFI firmware loader must handle pre-allocated buffer
>   fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
>   fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
>   module: Add hook for security_kernel_post_read_file()

Looks good to me, thanks for fixing this up:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Mimi Zohar]

Hi Kees,

On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> Hi,
> 
> In looking for closely at the additions that got made to the
> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> *kinds* of files for the LSM to reason about. They are a "how" and
> "where", respectively. Remove these improper aliases and refactor the
> code to adapt to the changes.

Thank you for adding the missing calls and the firmware pre allocated
buffer comment update.

> 
> Additionally adds in missing calls to security_kernel_post_read_file()
> in the platform firmware fallback path (to match the sysfs firmware
> fallback path) and in module loading. I considered entirely removing
> security_kernel_post_read_file() hook since it is technically unused,
> but IMA probably wants to be able to measure EFI-stored firmware images,
> so I wired it up and matched it for modules, in case anyone wants to
> move the module signature checks out of the module core and into an LSM
> to avoid the current layering violations.

IMa has always verified kernel module signatures.  Recently appended
kernel module signature support was added to IMA.  The same appended
signature format is also being used to sign and verify the kexec
kernel image.

With IMA's new kernel module appended signature support and patch 4/4
in this series, IMA won't be limit to the finit_module syscall, but
could support the init_module syscall as well.

> 
> This touches several trees, and I suspect it would be best to go through
> James's LSM tree.

Sure.

thanks!

Mimi

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

On 2020-07-07 1:19 a.m., Kees Cook wrote:
> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> that are interested in filtering between types of things. The "how"
> should be an internal detail made uninteresting to the LSMs.
>
> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   drivers/base/firmware_loader/main.c | 5 ++---
>   fs/exec.c                           | 7 ++++---
>   include/linux/fs.h                  | 2 +-
>   security/integrity/ima/ima_main.c   | 6 ++----
>   4 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b13524e..c2f57cedcd6f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>   	int i, len;
>   	int rc = -ENOENT;
>   	char *path;
> -	enum kernel_read_file_id id = READING_FIRMWARE;
>   	size_t msize = INT_MAX;
>   	void *buffer = NULL;
>   
>   	/* Already populated data member means we're loading into a buffer */
>   	if (!decompress && fw_priv->data) {
>   		buffer = fw_priv->data;
> -		id = READING_FIRMWARE_PREALLOC_BUFFER;
>   		msize = fw_priv->allocated_size;
>   	}
>   
> @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>   
>   		/* load firmware files from the mount namespace of init */
>   		rc = kernel_read_file_from_path_initns(path, &buffer,
> -						       &size, msize, id);
> +						       &size, msize,
> +						       READING_FIRMWARE);
>   		if (rc) {
>   			if (rc != -ENOENT)
>   				dev_warn(device, "loading %s failed with error %d\n",
> diff --git a/fs/exec.c b/fs/exec.c
> index e6e8a9a70327..2bf549757ce7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   {
>   	loff_t i_size, pos;
>   	ssize_t bytes = 0;
> +	void *allocated = NULL;
>   	int ret;
>   
>   	if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   		goto out;
>   	}
>   
> -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> -		*buf = vmalloc(i_size);
> +	if (!*buf)
> +		*buf = allocated = vmalloc(i_size);
>   	if (!*buf) {
>   		ret = -ENOMEM;
>   		goto out;
> @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   
>   out_free:
>   	if (ret < 0) {
> -		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +		if (allocated) {
>   			vfree(*buf);
>   			*buf = NULL;
>   		}
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3f881a892ea7..95fc775ed937 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
>   #endif
>   extern int do_pipe_flags(int *, int);
>   
> +/* This is a list of *what* is being read, not *how*. */
>   #define __kernel_read_file_id(id) \
>   	id(UNKNOWN, unknown)		\
>   	id(FIRMWARE, firmware)		\
With this change, I'm trying to figure out how the partial firmware read 
is going to work on top of this reachitecture.
Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a 
"what"?
> -	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
My patch series gets rejected any time I make a change to the 
kernel_read_file* region in linux/fs.h.
The requirement is for this api to move to another header file outside 
of linux/fs.h
It seems the same should apply to your change.
Could you please add the following patch to the start of you patch 
series to move the kernel_read_file* to its own include file?
https://patchwork.kernel.org/patch/11647063/
>   	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
>   	id(MODULE, kernel-module)		\
>   	id(KEXEC_IMAGE, kexec-image)		\
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..f80ee4ce4669 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
>   int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>   {
>   	/*
> -	 * READING_FIRMWARE_PREALLOC_BUFFER
> -	 *
>   	 * Do devices using pre-allocated memory run the risk of the
>   	 * firmware being accessible to the device prior to the completion
>   	 * of IMA's signature verification any more than when using two
> -	 * buffers?
> +	 * buffers? It may be desirable to include the buffer address
> +	 * in this API and walk all the dma_map_single() mappings to check.
>   	 */
>   	return 0;
>   }
>   
>   const int read_idmap[READING_MAX_ID] = {
>   	[READING_FIRMWARE] = FIRMWARE_CHECK,
> -	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
>   	[READING_MODULE] = MODULE_CHECK,
>   	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
>   	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Kees Cook]

On Tue, Jul 07, 2020 at 11:36:04AM -0400, Mimi Zohar wrote:
> Hi Kees,
> 
> On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > Hi,
> > 
> > In looking for closely at the additions that got made to the
> > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > *kinds* of files for the LSM to reason about. They are a "how" and
> > "where", respectively. Remove these improper aliases and refactor the
> > code to adapt to the changes.
> 
> Thank you for adding the missing calls and the firmware pre allocated
> buffer comment update.
> 
> > 
> > Additionally adds in missing calls to security_kernel_post_read_file()
> > in the platform firmware fallback path (to match the sysfs firmware
> > fallback path) and in module loading. I considered entirely removing
> > security_kernel_post_read_file() hook since it is technically unused,
> > but IMA probably wants to be able to measure EFI-stored firmware images,
> > so I wired it up and matched it for modules, in case anyone wants to
> > move the module signature checks out of the module core and into an LSM
> > to avoid the current layering violations.
> 
> IMa has always verified kernel module signatures.  Recently appended

Right, but not through the kernel_post_read_file() hook, nor via
out-of-band hooks in kernel/module.c. I was just meaning that future
work could be done here to regularize module_sig_check() into an actual
LSM (which could, in theory, be extended to kexec() to avoid code
duplication there, as kimage_validate_signature() has some overlap with
mod_verify_sig()). into a bit more normal of an LSM.

As far as IMA and regularizing things, though, what about fixing IMA to
not manually stack:

$ grep -B3 ima_ security/security.c
        ret = call_int_hook(bprm_check_security, 0, bprm);
        if (ret)
                return ret;
        return ima_bprm_check(bprm);
--
        ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot);
        if (ret)
                return ret;
        return ima_file_mprotect(vma, prot);
...

Can IMA implement a hook-last method to join the regular stacking routines?

> kernel module signature support was added to IMA.  The same appended
> signature format is also being used to sign and verify the kexec
> kernel image.
> 
> With IMA's new kernel module appended signature support and patch 4/4
> in this series, IMA won't be limit to the finit_module syscall, but
> could support the init_module syscall as well.

Exactly.

> > This touches several trees, and I suspect it would be best to go through
> > James's LSM tree.
> 
> Sure.

Is this an "Acked-by"? :)

-- 
Kees Cook

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Kees Cook]

On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote:
> On 2020-07-07 1:19 a.m., Kees Cook wrote:
> > FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> > that are interested in filtering between types of things. The "how"
> > should be an internal detail made uninteresting to the LSMs.
> > 
> > Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> > Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> > Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> > [...]
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3f881a892ea7..95fc775ed937 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
> >   #endif
> >   extern int do_pipe_flags(int *, int);
> > +/* This is a list of *what* is being read, not *how*. */
> >   #define __kernel_read_file_id(id) \
> >   	id(UNKNOWN, unknown)		\
> >   	id(FIRMWARE, firmware)		\
> With this change, I'm trying to figure out how the partial firmware read is
> going to work on top of this reachitecture.
> Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a
> "what"?

No, that's why I said you need to do the implementation within the API
and not expect each LSM to implement their own (as I mentioned both
times):

https://lore.kernel.org/lkml/202005221551.5CA1372@keescook/
https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/

I will reply in the thread above.

> > -	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
> My patch series gets rejected any time I make a change to the
> kernel_read_file* region in linux/fs.h.
> The requirement is for this api to move to another header file outside of
> linux/fs.h
> It seems the same should apply to your change.

Well I'm hardly making the same level of changes, but yeah, sure, if
that helps move things along, I can include that here.

> Could you please add the following patch to the start of you patch series to
> move the kernel_read_file* to its own include file?
> https://patchwork.kernel.org/patch/11647063/

https://lore.kernel.org/lkml/20200706232309.12010-2-scott.branden@broadcom.com/

You've included it in include/linux/security.h and that should be pretty
comprehensive, it shouldn't be needed in so many .c files.

-- 
Kees Cook

Re: [PATCH 4/4] module: Add hook for security_kernel_post_read_file()

[Mimi Zohar]

On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> Calls to security_kernel_load_data() should be paired with a call to
> security_kernel_post_read_file() with a NULL file argument. Add the
> missing call so the module contents are visible to the LSMs interested
> in measuring the module content. (This also paves the way for moving
> module signature checking out of the module core and into an LSM.)
> 
> Cc: Jessica Yu <jeyu@kernel.org>
> Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/module.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 0c6573b98c36..af9679f8e5c6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  		return -EFAULT;
>  	}
>  
> -	return 0;
> +	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> +					     info->len, READING_MODULE);

There was a lot of push back on calling security_kernel_read_file()
with a NULL file descriptor here.[1]  The result was defining a new
security hook - security_kernel_load_data - and enumeration -
LOADING_MODULE.  I would prefer calling the same pre and post security
hook.

Mimi

[1] http://kernsec.org/pipermail/linux-security-module-archive/2018-Ma
y/007110.html

> +	if (err)
> +		vfree(info->hdr);
> +
> +	return err;
>  }
>  
>  static void free_copy(struct load_info *info)

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

Hi Kees,

Thanks for looking at my patch series to see how it relates.
I see what you're trying to accomplish in various areas of cleanup.
I'll comment as I go through your individual emails.
1 comment below.

On 2020-07-07 2:55 p.m., Kees Cook wrote:
> On Tue, Jul 07, 2020 at 09:42:02AM -0700, Scott Branden wrote:
>> On 2020-07-07 1:19 a.m., Kees Cook wrote:
>>> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
>>> that are interested in filtering between types of things. The "how"
>>> should be an internal detail made uninteresting to the LSMs.
>>>
>>> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
>>> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
>>> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
>>> [...]
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index 3f881a892ea7..95fc775ed937 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
>>>    #endif
>>>    extern int do_pipe_flags(int *, int);
>>> +/* This is a list of *what* is being read, not *how*. */
>>>    #define __kernel_read_file_id(id) \
>>>    	id(UNKNOWN, unknown)		\
>>>    	id(FIRMWARE, firmware)		\
>> With this change, I'm trying to figure out how the partial firmware read is
>> going to work on top of this reachitecture.
>> Is it going to be ok to add READING_PARTIAL_FIRMWARE here as that is a
>> "what"?
> No, that's why I said you need to do the implementation within the API
> and not expect each LSM to implement their own (as I mentioned both
> times):
>
> https://lore.kernel.org/lkml/202005221551.5CA1372@keescook/
> https://lore.kernel.org/lkml/202007061950.F6B3D9E6A@keescook/
>
> I will reply in the thread above.
>
>>> -	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
>> My patch series gets rejected any time I make a change to the
>> kernel_read_file* region in linux/fs.h.
>> The requirement is for this api to move to another header file outside of
>> linux/fs.h
>> It seems the same should apply to your change.
> Well I'm hardly making the same level of changes, but yeah, sure, if
> that helps move things along, I can include that here.
>
>> Could you please add the following patch to the start of you patch series to
>> move the kernel_read_file* to its own include file?
>> https://patchwork.kernel.org/patch/11647063/
> https://lore.kernel.org/lkml/20200706232309.12010-2-scott.branden@broadcom.com/
>
> You've included it in include/linux/security.h and that should be pretty
> comprehensive, it shouldn't be needed in so many .c files.
Some people want the header files included in each c file they are used.
Others want header files not included if they are included in another 
header file.
I chose the first approach: every file that uses the api includes the 
header file.
I didn't know there was a standard approach to only put it in security.h
>

Re: [PATCH 4/4] module: Add hook for security_kernel_post_read_file()

[Kees Cook]

On Tue, Jul 07, 2020 at 08:47:20PM -0400, Mimi Zohar wrote:
> On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > Calls to security_kernel_load_data() should be paired with a call to
> > security_kernel_post_read_file() with a NULL file argument. Add the
> > missing call so the module contents are visible to the LSMs interested
> > in measuring the module content. (This also paves the way for moving
> > module signature checking out of the module core and into an LSM.)
> > 
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  kernel/module.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 0c6573b98c36..af9679f8e5c6 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> >  		return -EFAULT;
> >  	}
> >  
> > -	return 0;
> > +	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> > +					     info->len, READING_MODULE);
> 
> There was a lot of push back on calling security_kernel_read_file()
> with a NULL file descriptor here.[1]  The result was defining a new
> security hook - security_kernel_load_data - and enumeration -
> LOADING_MODULE.  I would prefer calling the same pre and post security
> hook.
> 
> Mimi
> 
> [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/007110.html

Ah yes, thanks for the pointer to the discussion.

I think we have four cases then, for differing LSM hooks:

- no "file", no contents
	e.g. init_module() before copying user buffer
	security_kernel_load_data()
- only a "file" available, no contents
	e.g. kernel_read_file() before actually reading anything
	security_kernel_read_file()
- "file" and contents
	e.g. kernel_read_file() after reading
	security_kernel_post_read_file()
- no "file" available, just the contents
	e.g. firmware platform fallback from EFI space (no "file")
	unimplemented!

If an LSM wants to be able to examine the contents of firmware, modules,
kexec, etc, it needs either a "file" or the full contents.

The "file" methods all pass through the kernel_read_file()-family. The
others happen via blobs coming from userspace or (more recently) the EFI
universe.

So, if a NULL file is unreasonable, we need, perhaps,
security_kernel_post_load_data()

?

-- 
Kees Cook

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Kees Cook]

On Tue, Jul 07, 2020 at 08:06:23PM -0700, Scott Branden wrote:
> Some people want the header files included in each c file they are used.
> Others want header files not included if they are included in another header
> file.

Ah, yes. That's fine then, leave them in the .c files.

-- 
Kees Cook

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Hans de Goede]

Hi,

On 7/7/20 10:19 AM, Kees Cook wrote:
> Hi,
> 
> In looking for closely at the additions that got made to the
> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> *kinds* of files for the LSM to reason about. They are a "how" and
> "where", respectively. Remove these improper aliases and refactor the
> code to adapt to the changes.
> 
> Additionally adds in missing calls to security_kernel_post_read_file()
> in the platform firmware fallback path (to match the sysfs firmware
> fallback path) and in module loading. I considered entirely removing
> security_kernel_post_read_file() hook since it is technically unused,
> but IMA probably wants to be able to measure EFI-stored firmware images,
> so I wired it up and matched it for modules, in case anyone wants to
> move the module signature checks out of the module core and into an LSM
> to avoid the current layering violations.
> 
> This touches several trees, and I suspect it would be best to go through
> James's LSM tree.
> 
> Thanks!


I've done some quick tests on this series to make sure that
the efi embedded-firmware support did not regress.
That still works fine, so this series is;

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> 
> -Kees
> 
> Kees Cook (4):
>    firmware_loader: EFI firmware loader must handle pre-allocated buffer
>    fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
>    fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
>    module: Add hook for security_kernel_post_read_file()
> 
>   drivers/base/firmware_loader/fallback_platform.c | 12 ++++++++++--
>   drivers/base/firmware_loader/main.c              |  5 ++---
>   fs/exec.c                                        |  7 ++++---
>   include/linux/fs.h                               |  3 +--
>   include/linux/lsm_hooks.h                        |  6 +++++-
>   kernel/module.c                                  |  7 ++++++-
>   security/integrity/ima/ima_main.c                |  6 ++----
>   7 files changed, 30 insertions(+), 16 deletions(-)
> 

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Hans de Goede]

Hi,

On 7/8/20 1:01 PM, Hans de Goede wrote:
> Hi,
> 
> On 7/7/20 10:19 AM, Kees Cook wrote:
>> Hi,
>>
>> In looking for closely at the additions that got made to the
>> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
>> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
>> *kinds* of files for the LSM to reason about. They are a "how" and
>> "where", respectively. Remove these improper aliases and refactor the
>> code to adapt to the changes.
>>
>> Additionally adds in missing calls to security_kernel_post_read_file()
>> in the platform firmware fallback path (to match the sysfs firmware
>> fallback path) and in module loading. I considered entirely removing
>> security_kernel_post_read_file() hook since it is technically unused,
>> but IMA probably wants to be able to measure EFI-stored firmware images,
>> so I wired it up and matched it for modules, in case anyone wants to
>> move the module signature checks out of the module core and into an LSM
>> to avoid the current layering violations.
>>
>> This touches several trees, and I suspect it would be best to go through
>> James's LSM tree.
>>
>> Thanks!
> 
> 
> I've done some quick tests on this series to make sure that
> the efi embedded-firmware support did not regress.
> That still works fine, so this series is;
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

I made a mistake during testing I was not actually running the
kernel with the patches added.

After fixing that I did find a problem, patch 4/4:
"module: Add hook for security_kernel_post_read_file()"

Breaks module-loading for me. This is with the 4 patches
on top of 5.8.0-rc4, so this might just be because I'm
not using the right base.

With patch 4/4 reverted things work fine for me.

So, please only add my Tested-by to patches 1-3.

Regards,

Hans



>> Kees Cook (4):
>>    firmware_loader: EFI firmware loader must handle pre-allocated buffer
>>    fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
>>    fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
>>    module: Add hook for security_kernel_post_read_file()
>>
>>   drivers/base/firmware_loader/fallback_platform.c | 12 ++++++++++--
>>   drivers/base/firmware_loader/main.c              |  5 ++---
>>   fs/exec.c                                        |  7 ++++---
>>   include/linux/fs.h                               |  3 +--
>>   include/linux/lsm_hooks.h                        |  6 +++++-
>>   kernel/module.c                                  |  7 ++++++-
>>   security/integrity/ima/ima_main.c                |  6 ++----
>>   7 files changed, 30 insertions(+), 16 deletions(-)
>>

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Luis Chamberlain]

On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/8/20 1:01 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 7/7/20 10:19 AM, Kees Cook wrote:
> > > Hi,
> > > 
> > > In looking for closely at the additions that got made to the
> > > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > > *kinds* of files for the LSM to reason about. They are a "how" and
> > > "where", respectively. Remove these improper aliases and refactor the
> > > code to adapt to the changes.
> > > 
> > > Additionally adds in missing calls to security_kernel_post_read_file()
> > > in the platform firmware fallback path (to match the sysfs firmware
> > > fallback path) and in module loading. I considered entirely removing
> > > security_kernel_post_read_file() hook since it is technically unused,
> > > but IMA probably wants to be able to measure EFI-stored firmware images,
> > > so I wired it up and matched it for modules, in case anyone wants to
> > > move the module signature checks out of the module core and into an LSM
> > > to avoid the current layering violations.
> > > 
> > > This touches several trees, and I suspect it would be best to go through
> > > James's LSM tree.
> > > 
> > > Thanks!
> > 
> > 
> > I've done some quick tests on this series to make sure that
> > the efi embedded-firmware support did not regress.
> > That still works fine, so this series is;
> > 
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> I made a mistake during testing I was not actually running the
> kernel with the patches added.
> 
> After fixing that I did find a problem, patch 4/4:
> "module: Add hook for security_kernel_post_read_file()"
> 
> Breaks module-loading for me. This is with the 4 patches
> on top of 5.8.0-rc4, so this might just be because I'm
> not using the right base.
> 
> With patch 4/4 reverted things work fine for me.
> 
> So, please only add my Tested-by to patches 1-3.

BTW is there any testing covered by the selftests for the firmware
laoder which would have caputured this? If not can you extend
it with something to capture this case you ran into?

  Luis

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Hans de Goede]

Hi,

On 7/8/20 1:55 PM, Luis Chamberlain wrote:
> On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/8/20 1:01 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 7/7/20 10:19 AM, Kees Cook wrote:
>>>> Hi,
>>>>
>>>> In looking for closely at the additions that got made to the
>>>> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
>>>> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
>>>> *kinds* of files for the LSM to reason about. They are a "how" and
>>>> "where", respectively. Remove these improper aliases and refactor the
>>>> code to adapt to the changes.
>>>>
>>>> Additionally adds in missing calls to security_kernel_post_read_file()
>>>> in the platform firmware fallback path (to match the sysfs firmware
>>>> fallback path) and in module loading. I considered entirely removing
>>>> security_kernel_post_read_file() hook since it is technically unused,
>>>> but IMA probably wants to be able to measure EFI-stored firmware images,
>>>> so I wired it up and matched it for modules, in case anyone wants to
>>>> move the module signature checks out of the module core and into an LSM
>>>> to avoid the current layering violations.
>>>>
>>>> This touches several trees, and I suspect it would be best to go through
>>>> James's LSM tree.
>>>>
>>>> Thanks!
>>>
>>>
>>> I've done some quick tests on this series to make sure that
>>> the efi embedded-firmware support did not regress.
>>> That still works fine, so this series is;
>>>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> I made a mistake during testing I was not actually running the
>> kernel with the patches added.
>>
>> After fixing that I did find a problem, patch 4/4:
>> "module: Add hook for security_kernel_post_read_file()"
>>
>> Breaks module-loading for me. This is with the 4 patches
>> on top of 5.8.0-rc4, so this might just be because I'm
>> not using the right base.
>>
>> With patch 4/4 reverted things work fine for me.
>>
>> So, please only add my Tested-by to patches 1-3.
> 
> BTW is there any testing covered by the selftests for the firmware
> laoder which would have caputured this? If not can you extend
> it with something to capture this case you ran into?

This was not a firmware-loading issue. For me in my tests,
which were limited to 1 device, patch 4/4, which only touches
the module-loading code, stopped module loading from working.

Since my test device has / on an eMMC and the kernel config
I'm using has mmc-block as a module, things just hung in the
initrd since no modules could be loaded, so I did not debug
this any further. Dropping  patch 4/4 from my local tree
solved this.

Regards,

Hans

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Luis Chamberlain]

On Wed, Jul 08, 2020 at 01:58:47PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/8/20 1:55 PM, Luis Chamberlain wrote:
> > On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 7/8/20 1:01 PM, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 7/7/20 10:19 AM, Kees Cook wrote:
> > > > > Hi,
> > > > > 
> > > > > In looking for closely at the additions that got made to the
> > > > > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > > > > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > > > > *kinds* of files for the LSM to reason about. They are a "how" and
> > > > > "where", respectively. Remove these improper aliases and refactor the
> > > > > code to adapt to the changes.
> > > > > 
> > > > > Additionally adds in missing calls to security_kernel_post_read_file()
> > > > > in the platform firmware fallback path (to match the sysfs firmware
> > > > > fallback path) and in module loading. I considered entirely removing
> > > > > security_kernel_post_read_file() hook since it is technically unused,
> > > > > but IMA probably wants to be able to measure EFI-stored firmware images,
> > > > > so I wired it up and matched it for modules, in case anyone wants to
> > > > > move the module signature checks out of the module core and into an LSM
> > > > > to avoid the current layering violations.
> > > > > 
> > > > > This touches several trees, and I suspect it would be best to go through
> > > > > James's LSM tree.
> > > > > 
> > > > > Thanks!
> > > > 
> > > > 
> > > > I've done some quick tests on this series to make sure that
> > > > the efi embedded-firmware support did not regress.
> > > > That still works fine, so this series is;
> > > > 
> > > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > > 
> > > I made a mistake during testing I was not actually running the
> > > kernel with the patches added.
> > > 
> > > After fixing that I did find a problem, patch 4/4:
> > > "module: Add hook for security_kernel_post_read_file()"
> > > 
> > > Breaks module-loading for me. This is with the 4 patches
> > > on top of 5.8.0-rc4, so this might just be because I'm
> > > not using the right base.
> > > 
> > > With patch 4/4 reverted things work fine for me.
> > > 
> > > So, please only add my Tested-by to patches 1-3.
> > 
> > BTW is there any testing covered by the selftests for the firmware
> > laoder which would have caputured this? If not can you extend
> > it with something to capture this case you ran into?
> 
> This was not a firmware-loading issue. For me in my tests,
> which were limited to 1 device, patch 4/4, which only touches
> the module-loading code, stopped module loading from working.
> 
> Since my test device has / on an eMMC and the kernel config
> I'm using has mmc-block as a module, things just hung in the
> initrd since no modules could be loaded, so I did not debug
> this any further. Dropping  patch 4/4 from my local tree
> solved this.

Thanks Hans!

Kees, would test_kmod.c and the respective selftest would have picked
this issue up?

  Luis

Re: [PATCH 4/4] module: Add hook for security_kernel_post_read_file()

[Mimi Zohar]

On Tue, 2020-07-07 at 20:10 -0700, Kees Cook wrote:
> On Tue, Jul 07, 2020 at 08:47:20PM -0400, Mimi Zohar wrote:
> > On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > > Calls to security_kernel_load_data() should be paired with a call to
> > > security_kernel_post_read_file() with a NULL file argument. Add the
> > > missing call so the module contents are visible to the LSMs interested
> > > in measuring the module content. (This also paves the way for moving
> > > module signature checking out of the module core and into an LSM.)
> > > 
> > > Cc: Jessica Yu <jeyu@kernel.org>
> > > Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > >  kernel/module.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 0c6573b98c36..af9679f8e5c6 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> > >  		return -EFAULT;
> > >  	}
> > >  
> > > -	return 0;
> > > +	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> > > +					     info->len, READING_MODULE);
> > 
> > There was a lot of push back on calling security_kernel_read_file()
> > with a NULL file descriptor here.[1]  The result was defining a new
> > security hook - security_kernel_load_data - and enumeration -
> > LOADING_MODULE.  I would prefer calling the same pre and post security
> > hook.
> > 
> > Mimi
> > 
> > [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/007110.html
> 
> Ah yes, thanks for the pointer to the discussion.
> 
> I think we have four cases then, for differing LSM hooks:
> 
> - no "file", no contents
> 	e.g. init_module() before copying user buffer
> 	security_kernel_load_data()
> - only a "file" available, no contents
> 	e.g. kernel_read_file() before actually reading anything
> 	security_kernel_read_file()
> - "file" and contents
> 	e.g. kernel_read_file() after reading
> 	security_kernel_post_read_file()
> - no "file" available, just the contents
> 	e.g. firmware platform fallback from EFI space (no "file")
> 	unimplemented!
> 
> If an LSM wants to be able to examine the contents of firmware, modules,
> kexec, etc, it needs either a "file" or the full contents.
> 
> The "file" methods all pass through the kernel_read_file()-family. The
> others happen via blobs coming from userspace or (more recently) the EFI
> universe.
> 
> So, if a NULL file is unreasonable, we need, perhaps,
> security_kernel_post_load_data()
> 
> ?

Agreed.

Mimi

Re: [PATCH 0/4] Fix misused kernel_read_file() enums

[Kees Cook]

On Wed, Jul 08, 2020 at 01:30:04PM +0000, Luis Chamberlain wrote:
> On Wed, Jul 08, 2020 at 01:58:47PM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 7/8/20 1:55 PM, Luis Chamberlain wrote:
> > > On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 7/8/20 1:01 PM, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 7/7/20 10:19 AM, Kees Cook wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > In looking for closely at the additions that got made to the
> > > > > > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > > > > > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > > > > > *kinds* of files for the LSM to reason about. They are a "how" and
> > > > > > "where", respectively. Remove these improper aliases and refactor the
> > > > > > code to adapt to the changes.
> > > > > > 
> > > > > > Additionally adds in missing calls to security_kernel_post_read_file()
> > > > > > in the platform firmware fallback path (to match the sysfs firmware
> > > > > > fallback path) and in module loading. I considered entirely removing
> > > > > > security_kernel_post_read_file() hook since it is technically unused,
> > > > > > but IMA probably wants to be able to measure EFI-stored firmware images,
> > > > > > so I wired it up and matched it for modules, in case anyone wants to
> > > > > > move the module signature checks out of the module core and into an LSM
> > > > > > to avoid the current layering violations.
> > > > > > 
> > > > > > This touches several trees, and I suspect it would be best to go through
> > > > > > James's LSM tree.
> > > > > > 
> > > > > > Thanks!
> > > > > 
> > > > > 
> > > > > I've done some quick tests on this series to make sure that
> > > > > the efi embedded-firmware support did not regress.
> > > > > That still works fine, so this series is;
> > > > > 
> > > > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > > > 
> > > > I made a mistake during testing I was not actually running the
> > > > kernel with the patches added.
> > > > 
> > > > After fixing that I did find a problem, patch 4/4:
> > > > "module: Add hook for security_kernel_post_read_file()"
> > > > 
> > > > Breaks module-loading for me. This is with the 4 patches
> > > > on top of 5.8.0-rc4, so this might just be because I'm
> > > > not using the right base.
> > > > 
> > > > With patch 4/4 reverted things work fine for me.
> > > > 
> > > > So, please only add my Tested-by to patches 1-3.
> > > 
> > > BTW is there any testing covered by the selftests for the firmware
> > > laoder which would have caputured this? If not can you extend
> > > it with something to capture this case you ran into?
> > 
> > This was not a firmware-loading issue. For me in my tests,
> > which were limited to 1 device, patch 4/4, which only touches
> > the module-loading code, stopped module loading from working.
> > 
> > Since my test device has / on an eMMC and the kernel config
> > I'm using has mmc-block as a module, things just hung in the
> > initrd since no modules could be loaded, so I did not debug
> > this any further. Dropping  patch 4/4 from my local tree
> > solved this.
> 
> Thanks Hans!
> 
> Kees, would test_kmod.c and the respective selftest would have picked
> this issue up?

I need to check -- I got a (possibly related) 0day report on it too.

Since I have to clean it up further based on Mimi's comments, and adapt
it a bit for Scott's series, I'll need to get a v2 spun for sure. :)

-- 
Kees Cook

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

Hi Kees,

This patch fails during booting of my system - see below.

On 2020-07-07 1:19 a.m., Kees Cook wrote:
> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> that are interested in filtering between types of things. The "how"
> should be an internal detail made uninteresting to the LSMs.
>
> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   drivers/base/firmware_loader/main.c | 5 ++---
>   fs/exec.c                           | 7 ++++---
>   include/linux/fs.h                  | 2 +-
>   security/integrity/ima/ima_main.c   | 6 ++----
>   4 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b13524e..c2f57cedcd6f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>   	int i, len;
>   	int rc = -ENOENT;
>   	char *path;
> -	enum kernel_read_file_id id = READING_FIRMWARE;
>   	size_t msize = INT_MAX;
>   	void *buffer = NULL;
>   
>   	/* Already populated data member means we're loading into a buffer */
>   	if (!decompress && fw_priv->data) {
>   		buffer = fw_priv->data;
> -		id = READING_FIRMWARE_PREALLOC_BUFFER;
>   		msize = fw_priv->allocated_size;
>   	}
>   
> @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>   
>   		/* load firmware files from the mount namespace of init */
>   		rc = kernel_read_file_from_path_initns(path, &buffer,
> -						       &size, msize, id);
> +						       &size, msize,
> +						       READING_FIRMWARE);
>   		if (rc) {
>   			if (rc != -ENOENT)
>   				dev_warn(device, "loading %s failed with error %d\n",
> diff --git a/fs/exec.c b/fs/exec.c
> index e6e8a9a70327..2bf549757ce7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   {
>   	loff_t i_size, pos;
>   	ssize_t bytes = 0;
> +	void *allocated = NULL;
>   	int ret;
>   
>   	if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   		goto out;
>   	}
>   
> -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> -		*buf = vmalloc(i_size);
> +	if (!*buf)
The assumption that *buf is always NULL when id != 
READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
I get unhandled page faults due to this change on boot.
> +		*buf = allocated = vmalloc(i_size);
>   	if (!*buf) {
>   		ret = -ENOMEM;
>   		goto out;
> @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>   
>   out_free:
>   	if (ret < 0) {
> -		if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> +		if (allocated) {
>   			vfree(*buf);
>   			*buf = NULL;
>   		}
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3f881a892ea7..95fc775ed937 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
>   #endif
>   extern int do_pipe_flags(int *, int);
>   
> +/* This is a list of *what* is being read, not *how*. */
>   #define __kernel_read_file_id(id) \
>   	id(UNKNOWN, unknown)		\
>   	id(FIRMWARE, firmware)		\
> -	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
>   	id(FIRMWARE_EFI_EMBEDDED, firmware)	\
>   	id(MODULE, kernel-module)		\
>   	id(KEXEC_IMAGE, kexec-image)		\
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..f80ee4ce4669 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
>   int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>   {
>   	/*
> -	 * READING_FIRMWARE_PREALLOC_BUFFER
> -	 *
>   	 * Do devices using pre-allocated memory run the risk of the
>   	 * firmware being accessible to the device prior to the completion
>   	 * of IMA's signature verification any more than when using two
> -	 * buffers?
> +	 * buffers? It may be desirable to include the buffer address
> +	 * in this API and walk all the dma_map_single() mappings to check.
>   	 */
>   	return 0;
>   }
>   
>   const int read_idmap[READING_MAX_ID] = {
>   	[READING_FIRMWARE] = FIRMWARE_CHECK,
> -	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
>   	[READING_MODULE] = MODULE_CHECK,
>   	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
>   	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Matthew Wilcox]

On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> >   		goto out;
> >   	}
> > -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > -		*buf = vmalloc(i_size);
> > +	if (!*buf)
> The assumption that *buf is always NULL when id !=
> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> I get unhandled page faults due to this change on boot.

Did it give you a stack backtrace?

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>>    		goto out;
>>>    	}
>>> -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>> -		*buf = vmalloc(i_size);
>>> +	if (!*buf)
>> The assumption that *buf is always NULL when id !=
>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>> I get unhandled page faults due to this change on boot.
> Did it give you a stack backtrace?
Yes, but there's no requirement that *buf need to be NULL when calling 
this function.
To fix my particular crash I added the following locally:

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char 
__user *, uargs, int, flags)
  {
      struct load_info info = { };
      loff_t size;
-    void *hdr;
+    void *hdr = NULL;
      int err;

      err = may_init_module();
>

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Kees Cook]

On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
> 
> 
> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > > >    		goto out;
> > > >    	}
> > > > -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > > > -		*buf = vmalloc(i_size);
> > > > +	if (!*buf)
> > > The assumption that *buf is always NULL when id !=
> > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> > > I get unhandled page faults due to this change on boot.
> > Did it give you a stack backtrace?
> Yes, but there's no requirement that *buf need to be NULL when calling this
> function.
> To fix my particular crash I added the following locally:
> 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
> __user *, uargs, int, flags)
>  {
>      struct load_info info = { };
>      loff_t size;
> -    void *hdr;
> +    void *hdr = NULL;
>      int err;
> 
>      err = may_init_module();
> > 
> 

Thanks for the diagnosis and fix! I haven't had time to cycle back
around to this series yet. Hopefully soon. :)

-- 
Kees Cook

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

Hi Kees,

On 2020-07-10 3:44 p.m., Kees Cook wrote:
> On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
>>
>> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
>>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>>>>     		goto out;
>>>>>     	}
>>>>> -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>>>> -		*buf = vmalloc(i_size);
>>>>> +	if (!*buf)
>>>> The assumption that *buf is always NULL when id !=
>>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>>>> I get unhandled page faults due to this change on boot.
>>> Did it give you a stack backtrace?
>> Yes, but there's no requirement that *buf need to be NULL when calling this
>> function.
>> To fix my particular crash I added the following locally:
>>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
>> __user *, uargs, int, flags)
>>   {
>>       struct load_info info = { };
>>       loff_t size;
>> -    void *hdr;
>> +    void *hdr = NULL;
>>       int err;
>>
>>       err = may_init_module();
> Thanks for the diagnosis and fix! I haven't had time to cycle back
> around to this series yet. Hopefully soon. :)
I don't consider this a complete fix as there may be other callers which 
do not initialize
the *buf param to NULL before calling kernel_read_file.

But, it does boot my system.  Also, I was able to make modifications for my
pread changes that pass (and the IMA works with IMA patch in my series 
is dropped completely with your changes in place).

So your changes work for me other than the hack needed above.
>

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Scott Branden]

Hi Kees

On 2020-07-10 3:44 p.m., Kees Cook wrote:
> On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
>>
>> On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
>>> On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
>>>>> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>>>>>     		goto out;
>>>>>     	}
>>>>> -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
>>>>> -		*buf = vmalloc(i_size);
>>>>> +	if (!*buf)
>>>> The assumption that *buf is always NULL when id !=
>>>> READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
>>>> I get unhandled page faults due to this change on boot.
>>> Did it give you a stack backtrace?
>> Yes, but there's no requirement that *buf need to be NULL when calling this
>> function.
>> To fix my particular crash I added the following locally:
>>
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
>> __user *, uargs, int, flags)
>>   {
>>       struct load_info info = { };
>>       loff_t size;
>> -    void *hdr;
>> +    void *hdr = NULL;
>>       int err;
>>
>>       err = may_init_module();
> Thanks for the diagnosis and fix! I haven't had time to cycle back
> around to this series yet. Hopefully soon. :)
>
In order to assist in your patchset I have combined it with my patch 
series here:
https://github.com/sbranden/linux/tree/kernel_read_file_for_kees

Please let me know if this matches your expectations for my patches or 
if there is something else I need to change.

Thanks,
Scott

Re: [PATCH 2/4] fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums

[Kees Cook]

On Thu, Jul 16, 2020 at 01:35:17PM -0700, Scott Branden wrote:
> On 2020-07-10 3:44 p.m., Kees Cook wrote:
> > On Fri, Jul 10, 2020 at 03:10:25PM -0700, Scott Branden wrote:
> > > 
> > > On 2020-07-10 3:04 p.m., Matthew Wilcox wrote:
> > > > On Fri, Jul 10, 2020 at 02:00:32PM -0700, Scott Branden wrote:
> > > > > > @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > > > > >     		goto out;
> > > > > >     	}
> > > > > > -	if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> > > > > > -		*buf = vmalloc(i_size);
> > > > > > +	if (!*buf)
> > > > > The assumption that *buf is always NULL when id !=
> > > > > READING_FIRMWARE_PREALLOC_BUFFER doesn't appear to be correct.
> > > > > I get unhandled page faults due to this change on boot.
> > > > Did it give you a stack backtrace?
> > > Yes, but there's no requirement that *buf need to be NULL when calling this
> > > function.
> > > To fix my particular crash I added the following locally:
> > > 
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -3989,7 +3989,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char
> > > __user *, uargs, int, flags)
> > >   {
> > >       struct load_info info = { };
> > >       loff_t size;
> > > -    void *hdr;
> > > +    void *hdr = NULL;
> > >       int err;
> > > 
> > >       err = may_init_module();
> > Thanks for the diagnosis and fix! I haven't had time to cycle back
> > around to this series yet. Hopefully soon. :)
> > 
> In order to assist in your patchset I have combined it with my patch series
> here:
> https://github.com/sbranden/linux/tree/kernel_read_file_for_kees
> 
> Please let me know if this matches your expectations for my patches or if
> there is something else I need to change.

Thanks! I was working on the next revision of this last night, and I'm
trying to get through today's email to finish it. I'll take a look!

-- 
Kees Cook