On 01/05/2016 09:28 PM, Trent Piepho wrote:

Thanks for the review. Comments inline.

> On Tue, 2016-01-05 at 09:11 +0100, Marc Kleine-Budde wrote:
>> +static int do_bootm_arm_fit(struct image_data *data)
>> +{
>> +	struct fit_handle *handle;
>> +	int ret;
>> +	unsigned long mem_free;
>> +	unsigned long mem_start, mem_size;
>> +
>> +	handle = fit_open(data->os_file, data->os_num, data->verbose);
>> +	if (!handle)
>> +		return -EINVAL;
>> +
>> +	ret = sdram_start_and_size(&mem_start, &mem_size);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* no support for custom load address */
>> +	data->os_address = mem_start + PAGE_ALIGN(handle->kernel_size * 4);
>> +	data->os_res = request_sdram_region("fit-kernel", data->os_address, handle->kernel_size);
>> +	if (!data->os_res) {
>> +		pr_err("Cannot request region 0x%08lx - 0x%08lx\n",
>> +				data->os_address, handle->kernel_size);
>> +		ret = -ENOMEM;
>> +		goto err_out;
>> +	}
>> +	memcpy((void *)data->os_res->start, handle->kernel, handle->kernel_size);
>> +
>> +	/*
>> +	 * Put oftree/initrd close behind compressed kernel image to avoid
>> +	 * placing it outside of the kernels lowmem.
>> +	 */
>> +	if (handle->initrd_size) {
>> +		data->initrd_address = PAGE_ALIGN(data->os_res->end + SZ_1M);
>> +		data->initrd_res = request_sdram_region("fit-initrd", data->initrd_address, handle->initrd_size);
>> +		if (!data->initrd_res) {
>> +			ret = -ENOMEM;
>> +			goto err_out;
>> +		}
>> +		memcpy((void *)data->initrd_res->start, handle->initrd, handle->initrd_size);
>> +	}
>> +
>> +	data->of_root_node = of_unflatten_dtb(handle->oftree);
>> +	if (!data->of_root_node) {
>> +		pr_err("unable to unflatten devicetree\n");
>> +		ret = -EINVAL;
>> +		goto err_out;
>> +	}
>> +
>> +	/*
>> +	 * Put devicetree right after initrd if present or after the kernel
>> +	 * if not.
>> +	 */
>> +	if (data->initrd_res)
>> +		mem_free = PAGE_ALIGN(data->initrd_res->end);
>> +	else
>> +		mem_free = PAGE_ALIGN(data->os_res->end + SZ_1M);
> Why the extra 1M?

Let's see if Jan remembers, he has coded this.

>> +
>> +	return __do_bootm_linux(data, mem_free, 0);
>> +
>> +err_out:
>> +	if (handle)
>> +		fit_close(handle);
> 
> handle can't be NULL, it's been dereferenced in every path that gets
> there.

correct. tnx.

> 
>> +	return ret;
>> +}
>> +
>> +static struct image_handler arm_fit_handler = {
> 
> Can this be const?

No, because it has a struct list_head embedded.

>> +        .name = "FIT image",
>> +        .bootm = do_bootm_arm_fit,
>> +        .filetype = filetype_oftree,
>> +};
>> +
>>  static struct binfmt_hook binfmt_aimage_hook = {
>>  	.type = filetype_aimage,
>>  	.exec = "bootm",
>> @@ -578,6 +650,8 @@ static int armlinux_register_image_handler(void)
>>  		register_image_handler(&aimage_handler);
>>  		binfmt_register(&binfmt_aimage_hook);
>>  	}
>> +	if (IS_BUILTIN(CONFIG_CMD_BOOTM_FITIMAGE))
>> +	        register_image_handler(&arm_fit_handler);
>>  	binfmt_register(&binfmt_arm_zimage_hook);
>>  	binfmt_register(&binfmt_barebox_hook);
>>  
>> diff --git a/commands/Kconfig b/commands/Kconfig
>> index 1743670ed33c..b89627209f5a 100644
>> --- a/commands/Kconfig
>> +++ b/commands/Kconfig
>> @@ -418,6 +418,14 @@ config CMD_BOOTM_AIMAGE
>>  	help
>>  	  Support using Android Images.
>>  
>> +config CMD_BOOTM_FITIMAGE
>> +	bool
>> +	prompt "FIT image support"
>> +	select FITIMAGE
>> +	depends on CMD_BOOTM && ARM
>> +	help
>> +	  Support using FIT Images.
> 
> Perhaps a link about FIT images or a pointer to a file in Documentation
> could go here?

OK.

>> +/*
>> + * The consistency of the FTD structure was already checked by of_unflatten_dtb()
>> + */
>> +static int fit_verify_signature(struct device_node *sig_node, void *fit)
>> +{
>> +	uint32_t hashed_strings_start, hashed_strings_size;
>> +	struct string_list inc_nodes, exc_props;
>> +	struct rsa_public_key key = {};
>> +	struct digest *digest;
>> +	int sig_len;
>> +	const char *algo_name, *key_name, *sig_value;
>> +	char *key_path;
>> +	struct device_node *key_node;
>> +	enum hash_algo algo;
>> +	void *hash;
>> +	int ret;
>> +
>> +	if (of_property_read_string(sig_node, "algo", &algo_name)) {
>> +		pr_err("algo not found\n");
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	if (strcmp(algo_name, "sha1,rsa2048") == 0) {
>> +		algo = HASH_ALGO_SHA1;
>> +	} else if (strcmp(algo_name, "sha256,rsa4096") == 0) {
>> +		algo = HASH_ALGO_SHA256;
>> +	} else	{
>> +		pr_err("unknown algo %s\n", algo_name);
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	digest = digest_alloc_by_algo(algo);
>> +	if (!digest) {
>> +		pr_err("unsupported algo %s\n", algo_name);
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	sig_value = of_get_property(sig_node, "value", &sig_len);
>> +	if (!sig_value) {
>> +		pr_err("signature value not found\n");
>> +		ret = -EINVAL;
>> +		goto out_free_digest;
>> +	}
>> +
>> +	if (of_property_read_string(sig_node, "key-name-hint", &key_name)) {
>> +		pr_err("key name not found\n");
>> +		ret = -EINVAL;
>> +		goto out_free_digest;
>> +	}
>> +	key_path = asprintf("/signature/key-%s", key_name);
> 
> If I understand this correctly, one computes a hash of part of the
> device tree and then verifies it against a signature, also in the device
> tree, using an RSA key, also in the device tree.
> 
> What's the point of that?  Isn't it basically a complex CRC check for
> internal consistency?

No, as the public key comes from the device tree that's already active
in barebox. In my use case it's linked into the barebox binary which
itself is signed again (and tested by the ROM code before executed).

>> +	if (!key_name) {
>> +		ret = -ENOMEM;
>> +		goto out_free_digest;
>> +	}
>> +	key_node = of_find_node_by_path(key_path);
>> +	free(key_path);
>> +	if (!key_node) {
>> +		pr_info("failed to find key node\n");
>> +		ret = -ENOENT;
>> +		goto out_free_digest;
>> +	}
>> +
>> +	ret = rsa_of_read_key(key_node, &key);
>> +	if (ret) {
>> +		pr_info("failed to read key\n");
>> +		ret = -ENOENT;
>> +		goto out_free_digest;
>> +	}
>> +
>> +	if (of_property_read_u32_index(sig_node, "hashed-strings", 0, &hashed_strings_start)) {
>> +		pr_err("%s: hashed-strings start not found\n", __func__);
>> +		ret = -EINVAL;
>> +		goto out_free_digest;
>> +	}
>> +	if (of_property_read_u32_index(sig_node, "hashed-strings", 1, &hashed_strings_size)) {
>> +		pr_err("%s: hashed-strings size not found\n", __func__);
>> +		ret = -EINVAL;
>> +		goto out_free_digest;
>> +	}
>> +
>> +	string_list_init(&inc_nodes);
>> +	string_list_init(&exc_props);
>> +
>> +	if (of_read_string_list(sig_node, "hashed-nodes", &inc_nodes)) {
>> +		pr_err("%s: hashed-nodes invalid\n", __func__);
>> +		ret = -EINVAL;
>> +		goto out_sl;
>> +	}
>> +
>> +	string_list_add(&exc_props, "data");
>> +
>> +	digest_init(digest);
>> +	ret = fit_digest(fit, digest, &inc_nodes, &exc_props, hashed_strings_start, hashed_strings_size);
>> +	hash = xzalloc(digest_length(digest));
>> +	digest_final(digest, hash);
>> +
>> +	ret = rsa_verify(&key, sig_value, sig_len, hash, algo);
>> +	if (ret) {
>> +		pr_info("sig BAD\n");
>> +		ret = CHECK_LEVEL_NONE;
> 
> Info level message should maybe be a bit more descriptive.

OK

> 
>> +	} else {
>> +		pr_info("sig OK\n");
>> +		ret = CHECK_LEVEL_SIG;
>> +	}
>> +
>> +	free(hash);
>> + out_sl:
>> +	string_list_free(&inc_nodes);
>> +	string_list_free(&exc_props);
>> + out_free_digest:
>> +	digest_free(digest);
>> + out:
>> +	return ret;
>> +}
>> +
>> +static int fit_verify_hash(struct device_node *hash, const void *data, int data_len)
>> +{
>> +	struct digest *d;
>> +	const char *algo;
>> +	const char *value_read;
>> +	char *value_calc;
>> +	int hash_len;
>> +
>> +	value_read = of_get_property(hash, "value", &hash_len);
>> +	if (!value_read) {
>> +		pr_err("value not found\n");
>> +		return CHECK_LEVEL_NONE;
> 
> Suggest adding node path to error messages.  "value not found" really
> doesn't tell anyone anything.

OK

> 
> This is an error message, but returns CHECK_LEVEL_NONE.  While later...
> 
>> +
>> +	if (memcmp(value_read, value_calc, hash_len)) {
>> +		pr_info("hash BAD\n");
>> +		digest_free(d);
>> +		return CHECK_LEVEL_NONE;
> 
> Now it's an info message and returns CHECK_LEVEL_NONE.  Seems wrong to
> return the same value for an error and a non-error.

I've kept the LEVEL_NONE here, but return with -EINVAL if no hash value
is found.

> 
>> +	} else {
>> +		pr_info("hash OK\n");
>> +		digest_free(d);
>> +		return CHECK_LEVEL_HASH;
>> +	}
>> +}
>> +
>> +static int fit_open_image(struct fit_handle *handle, const char* unit)
>> +{
>> +	struct device_node *image = NULL, *hash;
>> +	const char *type = NULL, *desc;
>> +	const void *data;
>> +	int data_len;
>> +	int ret, level;
>> +
>> +	image = of_get_child_by_name(handle->root, "images");
>> +	if (!image)
>> +		return -ENOENT;
>> +
>> +	image = of_get_child_by_name(image, unit);
>> +	if (!image)
>> +		return -ENOENT;
>> +
>> +	if (of_property_read_string(image, "description", &desc)) {
> Here you check the return value of of_property_read_string()
>> +		pr_info("FIT image '%s' (no description)\n", unit);
>> +	} else {
>> +		pr_info("FIT image '%s': '%s'\n", unit, desc);
>> +	}
> 
> Suggest:
>          desc = "(no description)";
>          of_property_read_string(image, "description", &desc);
>          pr_info("FIT image '%s': '%s'\n", unit, desc);
>          /* desc has valid value if anyone uses it again, instead of
>             being uninitialized */

tnx

>> +
>> +	of_property_read_string(image, "type", &type);
>> +	if (!type)
>> +		return -EINVAL;
> 
> But here you check that type is non-NULL.

It's later used in strcmp().

>> +
>> +	data = of_get_property(image, "data", &data_len);
>> +	if (!data) {
>> +		pr_err("data not found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	level = CHECK_LEVEL_MAX;
>> +	for_each_child_of_node(image, hash) {
>> +		if (handle->verbose)
>> +			of_print_nodes(hash, 0);
>> +		ret = fit_verify_hash(hash, data, data_len);
>> +		if (ret < 0)
>> +			return ret;
>> +		level = min(level, ret);
>> +	}
>> +	if (level == CHECK_LEVEL_MAX) {
>> +		return -EINVAL;
>> +	}
> Inconsistent use of {} for one statement then clause.

ok

>> +
>> +	if (level == CHECK_LEVEL_HASH) {
>> +		if (strcmp(type, "kernel") == 0 ||
>> +		    strcmp(type, "kernel_noload") == 0) {
>> +			handle->kernel = data;
>> +			handle->kernel_size = data_len;
>> +		} else if (strcmp(type, "flat_dt") == 0) {
>> +			handle->oftree = data;
>> +			handle->oftree_size = data_len;
>> +		} else if (strcmp(type, "ramdisk") == 0) {
>> +			handle->initrd = data;
>> +			handle->initrd_size = data_len;
>> +		} else {
>> +			pr_info("unknown image type %s, ignoring\n", type);
>> +		}
>> +	}
>> +
>> +	return level;
>> +}
>> +
>> +static int fit_open_configuration(struct fit_handle *handle, int num)
>> +{
>> +	struct device_node *conf_node = NULL, *sig_node;
>> +	char unit_name[10];
>> +	const char *unit, *desc;
>> +	int ret, level;
>> +
>> +	conf_node = of_get_child_by_name(handle->root, "configurations");
>> +	if (!conf_node)
>> +		return -ENOENT;
>> +
>> +	if (num) {
>> +		snprintf(unit_name, sizeof(unit_name), "conf@%d", num);
>> +		unit = unit_name;
>> +	} else if (of_property_read_string(conf_node, "default", &unit)) {
>> +		unit = "conf@1";
>> +	}
>> +
>> +	conf_node = of_get_child_by_name(conf_node, unit);
>> +	if (!conf_node) {
>> +		pr_err("FIT configuration '%s' not found\n", unit);
>> +		return -ENOENT;
>> +	}
>> +
>> +	if (of_property_read_string(conf_node, "description", &desc)) {
>> +		pr_info("FIT configuration '%s' (no description)\n", unit);
>> +	} else {
>> +		pr_info("FIT configuration '%s': '%s'\n", unit, desc);
>> +	}
>> +
>> +	level = CHECK_LEVEL_MAX;
>> +	for_each_child_of_node(conf_node, sig_node) {
>> +		if (handle->verbose)
>> +			of_print_nodes(sig_node, 0);
>> +		ret = fit_verify_signature(sig_node, handle->fit);
>> +		if (ret < 0)
>> +			return ret;
>> +		level = min(level, ret);
>> +	}
>> +	if (level == CHECK_LEVEL_MAX)
>> +		return -EINVAL;
> 
> This function up to here seems very similar to fit_open_image().  Could
> they be refactored, Ie. _fit_open(handle, "images", unit) vs
> _fit_open(handle, "configurations", "conf@1").

Given the fact that Yegor wants to improve the configuration selection
anyways I'd postpone this until he's finished.

> 
>> +
>> +	if (level != CHECK_LEVEL_SIG)
>> +		return -EINVAL;
>> +
>> +	if (of_property_read_string(conf_node, "kernel", &unit) == 0)
>> +		level = min(level, fit_open_image(handle, unit));
>> +	else
>> +		return -ENOENT;
>> +
>> +	if (of_property_read_string(conf_node, "fdt", &unit) == 0)
>> +		level = min(level, fit_open_image(handle, unit));
>> +
>> +	if (of_property_read_string(conf_node, "ramdisk", &unit) == 0)
>> +		level = min(level, fit_open_image(handle, unit));
>> +
>> +	if (level != CHECK_LEVEL_HASH)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +struct fit_handle *fit_open(const char *filename, int num, bool verbose)
>> +{
>> +	struct fit_handle *handle = NULL;
>> +	const char *desc;
>> +
>> +	handle = xzalloc(sizeof(struct fit_handle));
>> +
>> +	handle->verbose = verbose;
>> +
>> +	handle->fit = read_file(filename, &handle->size);
>> +	if (!handle->fit) {
>> +		pr_err("unable to read %s: %s\n", filename, strerror(errno));
>> +		goto err;
>> +	}
>> +
>> +	handle->root = of_unflatten_dtb(handle->fit);
>> +	if (IS_ERR(handle->root)) {
>> +		goto err;
>> +	}
> Use of {} for one statement if clauses is inconsistent in this function.

tnx

>> +
>> +	if (of_property_read_string(handle->root, "description", &desc)) {
>> +		pr_info("FIT '%s' (no description)\n", filename);
>> +	} else {
>> +		pr_info("FIT '%s': '%s'\n", filename, desc);
>> +	}
>> +
>> +	if (fit_open_configuration(handle, num))
>> +		goto err;
>> +
>> +	return handle;
>> + err:
>> +	if (handle->root)
>> +		of_delete_node(handle->root);
>> +	if (handle->fit)
>> +		free(handle->fit);
>> +	free(handle);
>> +
>> +	return NULL;
>> +}
>> +
>> +void fit_close(struct fit_handle *handle)
>> +{
>> +	if (handle->root)
>> +		of_delete_node(handle->root);
>> +	if (handle->fit)
>> +		free(handle->fit);
> 
> Isn't free(NULL) allowed in Barebox?  In the kernel it's defined that
> kfree() will check for NULL and code should not check before calling it.

tnx, should be no problem.

> 
>> +	free(handle);
>> +}
>> +
>> +#ifdef CONFIG_SANDBOX
>> +static int do_bootm_sandbox_fit(struct image_data *data)
>> +{
>> +	struct fit_handle *handle;
>> +	handle = fit_open(data->os_file, data->os_num, data->verbose);
>> +	if (handle)
>> +		fit_close(handle);
>> +	return 0;
>> +}
>> +
>> +static struct image_handler sandbox_fit_handler = {
>> +	.name = "FIT image",
>> +	.bootm = do_bootm_sandbox_fit,
>> +	.filetype = filetype_oftree,
>> +};
>> +
>> +static int sandbox_fit_register(void)
>> +{
>> +	return register_image_handler(&sandbox_fit_handler);
>> +}
>> +late_initcall(sandbox_fit_register);
>> +#endif
>> diff --git a/include/image-fit.h b/include/image-fit.h
>> new file mode 100644
>> index 000000000000..bcbc859ead37
>> --- /dev/null
>> +++ b/include/image-fit.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + *
>> + * Copyright (C) Jan Lübbe, 2014
>> + */
>> +
>> +#ifndef __IMAGE_FIT_H__
>> +#define __IMAGE_FIT_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct fit_handle {
>> +	void *fit;
>> +	size_t size;
>> +
>> +	bool verbose;
>> +
>> +	struct device_node *root;
>> +
>> +	const void *kernel;
>> +	unsigned long kernel_size;
>> +	const void *oftree;
>> +	unsigned long oftree_size;
>> +	const void *initrd;
>> +	unsigned long initrd_size;
>> +};
>> +
>> +struct fit_handle *fit_open(const char *filename, int num, bool verbose);
>> +void fit_close(struct fit_handle *handle);
>> +
>> +#endif	/* __IMAGE_FIT_H__ */
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |