Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly into DMA memory

[Mimi Zohar]

Hi Stephen,

[CC''ing linux-security-module]

On Tue, 2016-03-08 at 16:22 +0700, Stephen Boyd wrote:
> Some systems are memory constrained but they need to load very
> large firmwares. The firmware subsystem allows drivers to request
> this firmware be loaded from the filesystem, but this requires
> that the entire firmware be loaded into kernel memory first
> before it's provided to the driver. This can lead to a situation
> where we map the firmware twice, once to load the firmware into
> kernel memory and once to copy the firmware into the final
> resting place.
> 
> This design creates needless memory pressure and delays loading
> because we have to copy from kernel memory to somewhere else.
> Let's add a request_firmware_dma() API that allows drivers to
> request firmware be loaded directly into a DMA buffer that's been
> pre-allocated. This skips the intermediate step of allocating a
> buffer in kernel memory to hold the firmware image while it's
> read from the filesystem and copying it to another location. It
> also requires that drivers know how much memory they'll require
> before requesting the firmware and negates any benefits of
> firmware caching.

Prior to the kernel_read_file() family of functions, IMA/IMA-appraisal,
when enabled, pre-read the firmware a page at a time calculating the
file hash and then verified the file signature, before the firmware was
re-read into memory.   With the kernel_read_file() family of functions
the firmware is read into memory once, the signature verified, before
the driver is given access.

This patch set wants to read the file piecemeal, giving the driver
direct access to the pieces read, without first verifying the firmware
signature.  This breaks the concepts of measuring (trusted boot) or
appraising (secure boot) a file before usage.
   
> This is based on a patch from Vikram Mulukutla on
> codeaurora.org[1].
> 
> [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Vikram Mulukutla <markivx@xxxxxxxxxxxxxx>
> Cc: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Stephen Boyd <stephen.boyd@xxxxxxxxxx>
> ---
> 
> TODO:
>  * Handle security_kernel_fw_from_file() in the userhelper fallback case
>  * Rework for pending patches from Mimi Zohar that change the way files
>    are read in kernel space
> 
>  drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
>  include/linux/firmware.h      |  13 +++
>  2 files changed, 207 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 9a616cf7ac9f..8ab7a418f1d7 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,8 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/reboot.h>
>  #include <linux/security.h>
> +#include <linux/kernel.h>
> +#include <linux/dma-mapping.h>
> 
>  #include <generated/utsrelease.h>
> 
> @@ -154,6 +156,15 @@ struct firmware_buf {
>  	const char *fw_id;
>  };
> 
> +struct firmware_dma_desc {
> +	struct device *dev;
> +	void *cpu_addr;
> +	dma_addr_t dma_addr;
> +	unsigned long offset;
> +	size_t size;
> +	struct dma_attrs *attrs;
> +};
> +
>  struct fw_cache_entry {
>  	struct list_head list;
>  	const char *name;
> @@ -292,39 +303,83 @@ 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)
> +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
> +				 struct firmware_dma_desc *dma)
>  {
> -	int size;
> +	int size, rsize, remaining;
>  	char *buf;
>  	int rc;
> +	loff_t offset;
> +	unsigned long dma_offset;
> 
>  	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;
> +
> +	if (dma) {
> +		if (size + dma->offset > dma->size)
> +			return -EINVAL;
> +
> +		dma_offset = dma->offset;
> +		offset = 0;
> +		remaining = size;
> +
> +		while (remaining > 0) {
> +			rsize = min_t(int, remaining, PAGE_SIZE);
> +
> +			buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
> +					rsize, dma_offset, dma->attrs);
> +			if (!buf)
> +				return -ENOMEM;
> +
> +			rc = kernel_read(file, offset, buf, rsize);
> +			if (rc != rsize) {
> +				if (rc > 0)
> +					rc = -EIO;
> +				goto fail_dma;
> +			}
> +			rc = security_kernel_fw_from_file(file, buf, rsize); 

The security_kernel_fw_from_file hook was removed and replaced with the
pre and post hooks named security_kernel_read_file() hook and
security_kernel_post_read_file() respectively.  The pre security hook
gives the LSMs and IMA-appraisal opportunity to deny the read.   In
addition to giving the LSMs the opportunity to fail the firmware
request, the post security hook allows IMA/IMA-appraisal to measure the
file and appraise the file signature.

Mimi

> +			if (rc)
> +				goto fail_dma;
> +
> +			dma_unremap(dma->dev, buf, rsize, dma_offset,
> +				    dma->attrs);
> +			dma_offset += rsize;
> +			offset += rsize;
> +			remaining -= rsize;
> +		}
> +	} else {
> +		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;
>  	}
> -	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;
> +fail_dma:
> +	dma_unremap(dma->dev, buf, rsize, dma_offset, dma->attrs);
> +	return rc;
>  }
> 
> -static int fw_get_filesystem_firmware(struct device *device,
> -				       struct firmware_buf *buf)
> +static int
> +fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf,
> +			   struct firmware_dma_desc *dma)
>  {
>  	int i, len;
>  	int rc = -ENOENT;
> @@ -351,7 +406,7 @@ 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);
> +		rc = fw_read_file_contents(file, buf, dma);
>  		fput(file);
>  		if (rc)
>  			dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> @@ -470,6 +525,7 @@ struct firmware_priv {
>  	struct device dev;
>  	struct firmware_buf *buf;
>  	struct firmware *fw;
> +	struct firmware_dma_desc *dma;
>  };
> 
>  static struct firmware_priv *to_firmware_priv(struct device *dev)
> @@ -716,6 +772,52 @@ out:
> 
>  static DEVICE_ATTR(loading, 0644, firmware_loading_show, firmware_loading_store);
> 
> +static ssize_t firmware_dma_rw(struct firmware_dma_desc *dma, char *buffer,
> +			       loff_t *offset, size_t count, bool read)
> +{
> +	char *fw_buf;
> +	int retval = count;
> +
> +	if ((dma->offset + *offset + count) > dma->size)
> +		return -EINVAL;
> +
> +	fw_buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr, count,
> +			   dma->offset + *offset, dma->attrs);
> +	if (!fw_buf)
> +		return -ENOMEM;
> +
> +	if (read)
> +		memcpy(buffer, fw_buf, count);
> +	else
> +		memcpy(fw_buf, buffer, count);
> +
> +	dma_unremap(dma->dev, fw_buf, count, dma->offset + *offset, dma->attrs);
> +	*offset += count;
> +
> +	return retval;
> +}
> +
> +static void firmware_rw(struct firmware_buf *buf, char *buffer,
> +			loff_t offset, size_t count, bool read)
> +{
> +	while (count) {
> +		void *page_data;
> +		int page_nr = offset >> PAGE_SHIFT;
> +		int page_ofs = offset & (PAGE_SIZE-1);
> +		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> +
> +		page_data = kmap(buf->pages[page_nr]);
> +
> +		memcpy(buffer, page_data + page_ofs, page_cnt);
> +
> +		kunmap(buf->pages[page_nr]);
> +		buffer += page_cnt;
> +		offset += page_cnt;
> +		count -= page_cnt;
> +	}
> +
> +}
> +
>  static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
>  				  struct bin_attribute *bin_attr,
>  				  char *buffer, loff_t offset, size_t count)
> @@ -740,21 +842,12 @@ static ssize_t firmware_data_read(struct file *filp, struct kobject *kobj,
> 
>  	ret_count = count;
> 
> -	while (count) {
> -		void *page_data;
> -		int page_nr = offset >> PAGE_SHIFT;
> -		int page_ofs = offset & (PAGE_SIZE-1);
> -		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> -		page_data = kmap(buf->pages[page_nr]);
> +	if (fw_priv->dma)
> +		ret_count = firmware_dma_rw(fw_priv->dma, buffer, &offset,
> +					    count, true);
> +	else
> +		firmware_rw(buf, buffer, offset, count, true);
> 
> -		memcpy(buffer, page_data + page_ofs, page_cnt);
> -
> -		kunmap(buf->pages[page_nr]);
> -		buffer += page_cnt;
> -		offset += page_cnt;
> -		count -= page_cnt;
> -	}
>  out:
>  	mutex_unlock(&fw_lock);
>  	return ret_count;
> @@ -817,6 +910,7 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  {
>  	struct device *dev = kobj_to_dev(kobj);
>  	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> +	struct firmware_dma_desc *dma = fw_priv->dma;
>  	struct firmware_buf *buf;
>  	ssize_t retval;
> 
> @@ -830,26 +924,17 @@ static ssize_t firmware_data_write(struct file *filp, struct kobject *kobj,
>  		goto out;
>  	}
> 
> -	retval = fw_realloc_buffer(fw_priv, offset + count);
> -	if (retval)
> -		goto out;
> -
> -	retval = count;
> -
> -	while (count) {
> -		void *page_data;
> -		int page_nr = offset >> PAGE_SHIFT;
> -		int page_ofs = offset & (PAGE_SIZE - 1);
> -		int page_cnt = min_t(size_t, PAGE_SIZE - page_ofs, count);
> -
> -		page_data = kmap(buf->pages[page_nr]);
> -
> -		memcpy(page_data + page_ofs, buffer, page_cnt);
> +	if (dma) {
> +		retval = firmware_dma_rw(dma, buffer, &offset, count, false);
> +		if (retval < 0)
> +			goto out;
> +	} else {
> +		retval = fw_realloc_buffer(fw_priv, offset + count);
> +		if (retval)
> +			goto out;
> 
> -		kunmap(buf->pages[page_nr]);
> -		buffer += page_cnt;
> -		offset += page_cnt;
> -		count -= page_cnt;
> +		retval = count;
> +		firmware_rw(buf, buffer, offset, count, false);
>  	}
> 
>  	buf->size = max_t(size_t, offset, buf->size);
> @@ -887,7 +972,8 @@ static const struct attribute_group *fw_dev_attr_groups[] = {
> 
>  static struct firmware_priv *
>  fw_create_instance(struct firmware *firmware, const char *fw_name,
> -		   struct device *device, unsigned int opt_flags)
> +		   struct device *device, unsigned int opt_flags,
> +		   struct firmware_dma_desc *dma)
>  {
>  	struct firmware_priv *fw_priv;
>  	struct device *f_dev;
> @@ -900,6 +986,7 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
> 
>  	fw_priv->nowait = !!(opt_flags & FW_OPT_NOWAIT);
>  	fw_priv->fw = firmware;
> +	fw_priv->dma = dma;
>  	f_dev = &fw_priv->dev;
> 
>  	device_initialize(f_dev);
> @@ -920,7 +1007,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  	struct firmware_buf *buf = fw_priv->buf;
> 
>  	/* fall back on userspace loading */
> -	buf->is_paged_buf = true;
> +	if (!fw_priv->dma)
> +		buf->is_paged_buf = true;
> 
>  	dev_set_uevent_suppress(f_dev, true);
> 
> @@ -955,7 +1043,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
> 
>  	if (is_fw_load_aborted(buf))
>  		retval = -EAGAIN;
> -	else if (!buf->data)
> +	else if (buf->is_paged_buf && !buf->data)
>  		retval = -ENOMEM;
> 
>  	device_del(f_dev);
> @@ -966,11 +1054,12 @@ err_put_dev:
> 
>  static int fw_load_from_user_helper(struct firmware *firmware,
>  				    const char *name, struct device *device,
> -				    unsigned int opt_flags, long timeout)
> +				    unsigned int opt_flags, long timeout,
> +				    struct firmware_dma_desc *dma)
>  {
>  	struct firmware_priv *fw_priv;
> 
> -	fw_priv = fw_create_instance(firmware, name, device, opt_flags);
> +	fw_priv = fw_create_instance(firmware, name, device, opt_flags, dma);
>  	if (IS_ERR(fw_priv))
>  		return PTR_ERR(fw_priv);
> 
> @@ -998,7 +1087,7 @@ static void kill_requests_without_uevent(void)
>  static inline int
>  fw_load_from_user_helper(struct firmware *firmware, const char *name,
>  			 struct device *device, unsigned int opt_flags,
> -			 long timeout)
> +			 long timeout, struct firmware_dma_desc *dma)
>  {
>  	return -ENOENT;
>  }
> @@ -1119,7 +1208,8 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device,
>  /* called from request_firmware() and request_firmware_work_func() */
>  static int
>  _request_firmware(const struct firmware **firmware_p, const char *name,
> -		  struct device *device, unsigned int opt_flags)
> +		  struct device *device, struct firmware_dma_desc *dma,
> +		  unsigned int opt_flags)
>  {
>  	struct firmware *fw = NULL;
>  	long timeout;
> @@ -1156,7 +1246,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		}
>  	}
> 
> -	ret = fw_get_filesystem_firmware(device, fw->priv);
> +	ret = fw_get_filesystem_firmware(device, fw->priv, dma);
>  	if (ret) {
>  		if (!(opt_flags & FW_OPT_NO_WARN))
>  			dev_warn(device,
> @@ -1165,7 +1255,8 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>  		if (opt_flags & FW_OPT_USERHELPER) {
>  			dev_warn(device, "Falling back to user helper\n");
>  			ret = fw_load_from_user_helper(fw, name, device,
> -						       opt_flags, timeout);
> +						       opt_flags, timeout,
> +						       dma);
>  		}
>  	}
> 
> @@ -1212,7 +1303,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> 
>  	/* Need to pin this module until return */
>  	__module_get(THIS_MODULE);
> -	ret = _request_firmware(firmware_p, name, device,
> +	ret = _request_firmware(firmware_p, name, device, NULL,
>  				FW_OPT_UEVENT | FW_OPT_FALLBACK);
>  	module_put(THIS_MODULE);
>  	return ret;
> @@ -1236,7 +1327,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
>  	int ret;
> 
>  	__module_get(THIS_MODULE);
> -	ret = _request_firmware(firmware_p, name, device,
> +	ret = _request_firmware(firmware_p, name, device, NULL,
>  				FW_OPT_UEVENT | FW_OPT_NO_WARN);
>  	module_put(THIS_MODULE);
>  	return ret;
> @@ -1244,6 +1335,47 @@ int request_firmware_direct(const struct firmware **firmware_p,
>  EXPORT_SYMBOL_GPL(request_firmware_direct);
> 
>  /**
> + * request_firmware_dma - load firmware into a DMA allocated buffer
> + * @firmware_p: pointer to firmware image
> + * @name: name of firmware file
> + * @device: device for which firmware is being loaded and DMA region allocated
> + * @cpu_addr: address returned from dma_alloc_*()
> + * @dma_addr: address returned from dma_alloc_*()
> + * @offset: offset into DMA buffer to copy firmware into
> + * @size: size of DMA buffer
> + * @attrs: attributes used during DMA allocation time
> + *
> + * This function works pretty much like request_firmware(), but it doesn't
> + * load the firmware into @firmware_p's data member. Instead, the firmware
> + * is loaded directly into the buffer pointed to by @cpu_addr and @dma_addr.
> + * This function doesn't cache firmware either.
> + */
> +int
> +request_firmware_dma(const struct firmware **firmware_p, const char *name,
> +		     struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> +		     unsigned long offset, size_t size, struct dma_attrs *attrs)
> +{
> +	int ret;
> +	struct firmware_dma_desc dma = {
> +		.dev = device,
> +		.cpu_addr = cpu_addr,
> +		.dma_addr = dma_addr,
> +		.offset = offset,
> +		.size = size,
> +		.attrs = attrs,
> +	};
> +
> +	/* Need to pin this module until return */
> +	__module_get(THIS_MODULE);
> +	ret = _request_firmware(firmware_p, name, device, &dma,
> +				FW_OPT_UEVENT | FW_OPT_FALLBACK |
> +				FW_OPT_NOCACHE);
> +	module_put(THIS_MODULE);
> +	return ret;
> +}
> +EXPORT_SYMBOL(request_firmware_dma);
> +
> +/**
>   * release_firmware: - release the resource associated with a firmware image
>   * @fw: firmware resource to release
>   **/
> @@ -1275,7 +1407,7 @@ static void request_firmware_work_func(struct work_struct *work)
> 
>  	fw_work = container_of(work, struct firmware_work, work);
> 
> -	_request_firmware(&fw, fw_work->name, fw_work->device,
> +	_request_firmware(&fw, fw_work->name, fw_work->device, NULL,
>  			  fw_work->opt_flags);
>  	fw_work->cont(fw, fw_work->context);
>  	put_device(fw_work->device); /* taken in request_firmware_nowait() */
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e75b5c..06bc8d5965ca 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -19,6 +19,7 @@ struct firmware {
> 
>  struct module;
>  struct device;
> +struct dma_attrs;
> 
>  struct builtin_fw {
>  	char *name;
> @@ -47,6 +48,10 @@ int request_firmware_nowait(
>  	void (*cont)(const struct firmware *fw, void *context));
>  int request_firmware_direct(const struct firmware **fw, const char *name,
>  			    struct device *device);
> +int request_firmware_dma(const struct firmware **firmware_p, const char *name,
> +		     struct device *device, void *cpu_addr, dma_addr_t dma_addr,
> +		     unsigned long offset, size_t size,
> +		     struct dma_attrs *attrs);
> 
>  void release_firmware(const struct firmware *fw);
>  #else
> @@ -75,5 +80,13 @@ static inline int request_firmware_direct(const struct firmware **fw,
>  	return -EINVAL;
>  }
> 
> +static inline int request_firmware_dma(const struct firmware **firmware_p,
> +	const char *name, struct device *device, void *cpu_addr,
> +	dma_addr_t dma_addr, unsigned long offset, size_t size,
> +	struct dma_attrs *attrs)
> +{
> +	return -EINVAL;
> +}
> +
>  #endif
>  #endif