Re: [RFC PATCH v2 07/11] firmware: replace call to fw_read_file_contents() with kernel version

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Mon, Jan 18, 2016 at 10:11:22AM -0500, Mimi Zohar wrote:
> Replace fw_read_file_contents() for reading a file with the common VFS
> kernel_read_file() function.  A benefit of calling kernel_read_file()
> to read the firmware is the firmware is read only once, instead of once
> for measuring/appraising the firmware and again for reading the file
> contents into memory.
> 
> This patch retains the kernel_fw_from_file() hook, which is called from
> security_kernel_post_read_file(), but removes the
> sercurity_kernel_fw_from_file() function.
> 
> Changelog:
> - reordered and squashed firmware patches
> - fix MAX firmware size (Kees Cook)
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  drivers/base/firmware_class.c         | 48 ++++++++++-------------------------
>  include/linux/ima.h                   |  7 +----
>  include/linux/security.h              |  8 +-----
>  security/integrity/ima/ima.h          |  1 -
>  security/integrity/ima/ima_appraise.c |  8 ------
>  security/integrity/ima/ima_main.c     | 18 +++++--------
>  security/integrity/ima/ima_policy.c   | 26 +++++++++----------
>  security/integrity/integrity.h        | 11 +++-----
>  security/security.c                   | 28 ++++++++++----------
>  9 files changed, 54 insertions(+), 101 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450..cc175f1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,7 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/reboot.h>
>  #include <linux/security.h>
> +#include <linux/ima.h>
>  
>  #include <generated/utsrelease.h>
>  
> @@ -291,40 +292,10 @@ static const char * const fw_path[] = {
>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>  
> -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> -{
> -	int size;
> -	char *buf;
> -	int rc;
> -
> -	if (!S_ISREG(file_inode(file)->i_mode))
> -		return -EINVAL;
> -	size = i_size_read(file_inode(file));
> -	if (size <= 0)
> -		return -EINVAL;
> -	buf = vmalloc(size);
> -	if (!buf)
> -		return -ENOMEM;
> -	rc = kernel_read(file, 0, buf, size);
> -	if (rc != size) {
> -		if (rc > 0)
> -			rc = -EIO;
> -		goto fail;
> -	}
> -	rc = security_kernel_fw_from_file(file, buf, size);
> -	if (rc)
> -		goto fail;
> -	fw_buf->data = buf;
> -	fw_buf->size = size;
> -	return 0;
> -fail:
> -	vfree(buf);
> -	return rc;
> -}
> -
>  static int fw_get_filesystem_firmware(struct device *device,
>  				       struct firmware_buf *buf)
>  {
> +	loff_t size;
>  	int i, len;
>  	int rc = -ENOENT;
>  	char *path;
> @@ -350,13 +321,18 @@ static int fw_get_filesystem_firmware(struct device *device,
>  		file = filp_open(path, O_RDONLY, 0);
>  		if (IS_ERR(file))
>  			continue;
> -		rc = fw_read_file_contents(file, buf);
> +
> +		buf->size = 0;
> +		rc = kernel_read_file(file, &buf->data, &size, INT_MAX,
> +				      FIRMWARE_CHECK);

The way kernel firmware signing was implemented was that we'd first read the
foo.sig (or whatever extension we use). The same kernel_read_file() would be
used if this gets applied so this would still works well with that. Of course
folks using IMA and their own security policy would just disable the kernel
fw signing facility.

> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index ae91938..0a7f039 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -16,6 +16,7 @@ struct linux_binprm;
>  enum ima_policy_id {
>  	KEXEC_CHECK = 1,
>  	INITRAMFS_CHECK,
> +	FIRMWARE_CHECK,
>  	IMA_MAX_READ_CHECK
>  };

The only thing that is worth questioning here in light of kernel fw signing is
what int policy_id do we use? Would we be OK to add FIRMWARE_SIG_CHECK later 
While at it, perhaps kernel_read_file() last argument should be enum
ima_policy_id then. If the policy_id is going to be used elsewhere outside of
IMA then perhaps ima.h is not the best place for it ? Would fs.h for type of
file be OK ? Then we'd have a list of known file types the kernel reads.

  Luis