[PATCH v2 05/14] fip: rework fip_image_open()

[Marco Felsch]

On 25-03-11, Sascha Hauer wrote:
> fip_image_open() used to do all the parsing into a struct fip_state
> itself. Instead, only load the FIP image into a buffer and call
> fip_do_parse_buf() with this buffer. This has the advantage that we
> have all parsing of the FIP image in a single place. Also this helps
> with a followup patch which calculates a sha256 over a FIP image
> which can easily done when we have the whole FIP image in a contiguous
> buffer.
> 
> Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> ---
>  lib/fip.c | 78 +++++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 41 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/fip.c b/lib/fip.c
> index 23e82098da..aab2795476 100644
> --- a/lib/fip.c
> +++ b/lib/fip.c
> @@ -23,6 +23,7 @@
>  #include <string.h>
>  #include <libfile.h>
>  #include <fs.h>
> +#include <linux/kernel.h>
>  
>  #include <fip.h>
>  #include <fiptool.h>
> @@ -447,22 +448,17 @@ int fip_update(struct fip_state *fip)
>  }
>  
>  /*
> - * fip_image_open - open a FIP image for readonly access
> + * fip_image_open - open a FIP image
>   * @filename: The filename of the FIP image
>   * @offset: The offset of the FIP image in the file
>   *
> - * This opens a FIP image for readonly access. This is an alternative
> - * implementation for fip_parse() with these differences:
> + * This opens a FIP image. This is an alternative implementation for
> + * fip_parse() with these differences:
>   * - suitable for reading FIP images from raw partitions. This function
>   *   only reads the FIP image, even when the partition is bigger than the
>   *   image
>   * - Allows to specify an offset within the partition where the FIP image
>   *   starts
> - * - Do not memdup the images from the full FIP image
> - *
> - * This function is for easy readonly access to the images within the FIP
> - * image. Do not call any of the above FIP manipulation functions other than
> - * fip_free() on an image opened with this function.
>   */
>  struct fip_state *fip_image_open(const char *filename, size_t offset)
>  {
> @@ -470,11 +466,13 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
>  	int ret;
>  	int fd;
>  	struct fip_state *fip_state;
> -	LIST_HEAD(entries);
>  	size_t fip_headers_size, total = 0;
> -	struct fip_image_desc *desc;
>  	off_t pos;
>  	int n_entries = 0;
> +	struct fip_toc_entry toc_entries[16];
				 	 ^
Why did you used 16?

> +	void *buf, *ptr;
> +	int i;
> +	struct fip_toc_entry *toc_entry;
>  
>  	fd = open(filename, O_RDONLY);
>  	if (fd < 0)
> @@ -506,11 +504,9 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
>  
>  	/* read all toc entries */
>  	while (1) {
> -		struct fip_image_desc *desc = xzalloc(sizeof(*desc));
> -		struct fip_image *image = xzalloc(sizeof(*image));
> -		struct fip_toc_entry *toc_entry = &image->toc_e;
> +		uint64_t this_end;
>  
> -		desc->image = image;
> +		toc_entry = &toc_entries[n_entries];
>  
>  		ret = read_full(fd, toc_entry, sizeof(*toc_entry));
>  		if (ret < 0)
> @@ -520,53 +516,61 @@ struct fip_state *fip_image_open(const char *filename, size_t offset)
>  			goto err;
>  		}
>  
> -		list_add_tail(&desc->list, &fip_state->descs);
> -
>  		pr_debug("Read TOC entry %pU %llu %llu\n", &toc_entry->uuid,
>  			 toc_entry->offset_address, toc_entry->size);
>  
> -		/* Found the ToC terminator, we are done. */
> -		if (uuid_is_null(&toc_entry->uuid))
> -			break;
> -	}
> -
> -	/* determine buffer size */
> -	fip_for_each_desc(fip_state, desc) {
> -		uint64_t this_end = desc->image->toc_e.offset_address + desc->image->toc_e.size;
> +		this_end = toc_entry->offset_address + toc_entry->size;
>  
>  		if (this_end > total)
>  			total = this_end;
> +
>  		n_entries++;
> -	}
>  
> -	fip_headers_size = n_entries * sizeof(struct fip_toc_entry) + sizeof(fip_toc_header_t);
> +		if (n_entries >= ARRAY_SIZE(toc_entries)) {

			pr_err("Error: Only 16 toc-entries are supported at the moment\n");

> +			ret = -ENOSPC;
> +			goto err;
> +		}
>  
> -	total -= fip_headers_size;
> +		/* Found the ToC terminator, we are done. */
> +		if (uuid_is_null(&toc_entry->uuid))
> +			break;

If this check is happening after the n_entries check, we trigger a
false-positive in case of a 16-entries FIP.

Regards,
  Marco

> +	}
>  
> -	fip_state->buffer = malloc(total);
> -	if (!fip_state->buffer) {
> +	buf = malloc(total);
> +	if (!buf) {
>  		ret = -ENOMEM;
>  		goto err;
>  	}
>  
> -	ret = read_full(fd, fip_state->buffer, total);
> +	ptr = buf;
> +
> +	memcpy(ptr, &toc_header, sizeof(toc_header));
> +	ptr += sizeof(toc_header);
> +
> +	for (i = 0; i < n_entries; i++) {
> +		memcpy(ptr, &toc_entries[i], sizeof(*toc_entry));
> +		ptr += sizeof(*toc_entry);
> +	}
> +
> +	fip_headers_size = n_entries * sizeof(struct fip_toc_entry) + sizeof(fip_toc_header_t);
> +
> +	ret = read_full(fd, ptr, total - fip_headers_size);
>  	if (ret < 0)
>  		goto err;
>  
> -	if (ret < total) {
> +	if (ret < total - fip_headers_size) {
>  		ret = -ENODATA;
>  		goto err;
>  	}
>  
> -	close(fd);
> +	ret = fip_do_parse_buf(fip_state, buf, total, NULL);
> +	if (ret)
> +		goto err;
>  
> -	fip_for_each_desc(fip_state, desc) {
> -		desc->image->buffer = fip_state->buffer +
> -			desc->image->toc_e.offset_address - fip_headers_size;
> -		desc->image->buf_no_free = true;
> -	}
> +	close(fd);
>  
>  	return fip_state;
> +
>  err:
>  	close(fd);
>  	fip_free(fip_state);
> 
> -- 
> 2.39.5
> 
> 
>