Re: [RFC/PATCHv3 v3 3/3] firmware: Support loading into a pre-allocated buffer

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

Hi Stephen,

On Tue, 2016-05-10 at 15:26 -0700, Stephen Boyd wrote:

> -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)
>  {
>  	loff_t size;
>  	int i, len;
>  	int rc = -ENOENT;
>  	char *path;
> +	enum kernel_read_file_id id = READING_FIRMWARE;
> +	size_t msize = INT_MAX;
> +
> +	/* Already populated data member means we're loading into a buffer */
> +	if (buf->data) {
> +		id = READING_FIRMWARE_INTO_BUF;

In both cases we're reading the firmware into a buffer.  In this case,
it is pre-allocated.  Other than it being pre-allocated, is there
anything special about this buffer?  There has to be a more appropriate
string identifier.

> +		msize = buf->allocated_size;
> +	}
> 
>  	path = __getname();
>  	if (!path)

> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -836,8 +836,8 @@ int kernel_read(struct file *file, loff_t offset,
> 
>  EXPORT_SYMBOL(kernel_read);
> 
> -int kernel_read_file(struct file *file, void **buf, loff_t *size,
> -		     loff_t max_size, enum kernel_read_file_id id)
> +static int _kernel_read_file(struct file *file, void **buf, loff_t *size,
> +			     loff_t max_size, enum kernel_read_file_id id)
>  {
>  	loff_t i_size, pos;
>  	ssize_t bytes = 0;
> @@ -856,7 +856,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>  	if (i_size <= 0)
>  		return -EINVAL;
> 
> -	*buf = vmalloc(i_size);
> +	if (id != READING_FIRMWARE_INTO_BUF)
> +		*buf = vmalloc(i_size);
>  	if (!*buf)
>  		return -ENOMEM;
> 
> @@ -885,11 +886,19 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> 
>  out:
>  	if (ret < 0) {
> -		vfree(*buf);
> -		*buf = NULL;
> +		if (id != READING_FIRMWARE_INTO_BUF) {
> +			vfree(*buf);
> +			*buf = NULL;
> +		}
>  	}
>  	return ret;
>  }
> +
> +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> +		     loff_t max_size, enum kernel_read_file_id id)
> +{
> +	return _kernel_read_file(file, buf, size, max_size, id);
> +}
>  EXPORT_SYMBOL_GPL(kernel_read_file);

This seems to be exactly the same as the original version.

> 
>  int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> @@ -905,7 +914,7 @@ int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
>  	if (IS_ERR(file))
>  		return PTR_ERR(file);
> 
> -	ret = kernel_read_file(file, buf, size, max_size, id);
> +	ret = _kernel_read_file(file, buf, size, max_size, id);
>  	fput(file);
>  	return ret;
>  }
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 5c41c5e75b5c..bdc24ee92823 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -47,6 +47,8 @@ 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_into_buf(const struct firmware **firmware_p,
> +	const char *name, struct device *device, void *buf, size_t size);
> 
>  void release_firmware(const struct firmware *fw);
>  #else
> @@ -75,5 +77,11 @@ static inline int request_firmware_direct(const struct firmware **fw,
>  	return -EINVAL;
>  }
> 
> +static inline int request_firmware_into_buf(const struct firmware **firmware_p,
> +	const char *name, struct device *device, void *buf, size_t size);
> +{
> +	return -EINVAL;
> +}
> +
>  #endif
>  #endif
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 14a97194b34b..4fb8596b3dd4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2582,6 +2582,7 @@ extern int do_pipe_flags(int *, int);
> 
>  enum kernel_read_file_id {
>  	READING_FIRMWARE = 1,
> +	READING_FIRMWARE_INTO_BUF,
>  	READING_MODULE,
>  	READING_KEXEC_IMAGE,
>  	READING_KEXEC_INITRAMFS,

Commit "1284ab5 fs: define a string representation of the
kernel_read_file_id enumeration", which is queued to be upstreamed,
changes this a bit.

> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 60d011aaec38..b922d6ca2fb0 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -272,7 +272,8 @@ static ssize_t ima_read_policy(char *path)
>  	datap = path;
>  	strsep(&datap, "\n");
> 
> -	rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY);
> +	rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY,
> +					NULL);

It doesn't look like kernel_read_file_from_path() changed.

Mimi

>  	if (rc < 0) {
>  		pr_err("Unable to open file: %s (%d)", path, rc);
>  		return rc;