Re: [PATCH 3/3] bootm: add initial FIT support

[Yegor Yefremov]

On Thu, Jan 7, 2016 at 6:09 PM, Jan Lübbe  wrote:
> On Di, 2016-01-05 at 14:50 +0100, Marc Kleine-Budde wrote:
>> >> > +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);
>> >
>> > This is not working for my *.its file:
>> >
>> https://github.com/visionsystemsgmbh/onrisc_br_bsp/blob/master/board/vscom/baltos/kernel-fit.its
>> > U-Boot is working with bootm ${loadaddr}#conf${board_name}
>> >
>> > For Barebox I've changed this line to
>> >
>> > snprintf(unit_name, sizeof(unit_name), "conf%d@1", num)
>> >
>> > This is how I start Linux: bootm /boot/kernel-fit.itb@
>> $global.board.id
>> >
>> > What is the standard for providing FIT configuration?
>>
>> Don't know. Is there a spec in the u-boot sources, otherwise use the
>> code.
>
> I used the u-boot example ITS for reference (doc/uImage.FIT/multi.its).
> The have the number after the @, so I used the same. Barebox's bootm
> currently only supports a number to select between different "subimages"
> as a legacy from uImages.

But to be compatible with U-Boot Barebox should also accept strings
after bootm. See Joel's slides.

> Your ITS has a @1 for every config/fdt. Why?

I used http://elinux.org/images/f/f4/Elc2013_Fernandes.pdf creating my
*.its file. I don't know why, but Joel used @1 everywhere. I've
removed '@1' and it is working. As far as I understand you need this
really only to create unique nodes of the same "type".

> For selecting between different configurations depending on the board,
> it would be nice to be able to reuse the DT board compatibles.
>
> Regards,
> Jan
>
> --
> Pengutronix e.K.   | |
> Industrial Linux Solutions | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
> Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
>

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Jan Lübbe]

On Mi, 2016-01-06 at 17:09 +0100, Marc Kleine-Budde wrote:
[...]
> >> +  /*
> >> +   * 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.

This is the same as in arch/arm/lib/bootm.c. I wanted to avoid having
different behaviors.

Regards,
Jan
-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Jan Lübbe]

On Di, 2016-01-05 at 14:50 +0100, Marc Kleine-Budde wrote:
> >> > +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);
> > 
> > This is not working for my *.its file:
> >
> https://github.com/visionsystemsgmbh/onrisc_br_bsp/blob/master/board/vscom/baltos/kernel-fit.its
> > U-Boot is working with bootm ${loadaddr}#conf${board_name}
> > 
> > For Barebox I've changed this line to
> > 
> > snprintf(unit_name, sizeof(unit_name), "conf%d@1", num)
> > 
> > This is how I start Linux: bootm /boot/kernel-fit.itb@
> $global.board.id
> > 
> > What is the standard for providing FIT configuration?
> 
> Don't know. Is there a spec in the u-boot sources, otherwise use the
> code.

I used the u-boot example ITS for reference (doc/uImage.FIT/multi.its).
The have the number after the @, so I used the same. Barebox's bootm
currently only supports a number to select between different "subimages"
as a legacy from uImages.

Your ITS has a @1 for every config/fdt. Why?

For selecting between different configurations depending on the board,
it would be nice to be able to reuse the DT board compatibles.

Regards,
Jan

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Marc Kleine-Budde]

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(_start, _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(_handler);
>>  binfmt_register(_aimage_hook);
>>  }
>> +if (IS_BUILTIN(CONFIG_CMD_BOOTM_FITIMAGE))
>> +register_image_handler(_fit_handler);
>>  binfmt_register(_arm_zimage_hook);
>>  binfmt_register(_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", _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 = 

[PATCH 3/3] bootm: add initial FIT support

[Marc Kleine-Budde]

From: Jan Luebbe 

This implementation is inspired by U-Boot's FIT support. Instead of
using libfdt (which does not exist in barebox), configuration signatures
are verified by using a simplified DT parser based on barebox's own
code.

Currently, only signed configurations with hashed images are supported,
as the other variants are less useful for verified boot. Compatible FIT
images can be created using U-Boot's mkimage tool.

Signed-off-by: Jan Luebbe 
Signed-off-by: Marc Kleine-Budde 
---
 arch/arm/lib/bootm.c |  74 +++
 commands/Kconfig |   8 +
 common/Kconfig   |   6 +
 common/Makefile  |   1 +
 common/image-fit.c   | 590 +++
 include/image-fit.h  |  42 
 6 files changed, 721 insertions(+)
 create mode 100644 common/image-fit.c
 create mode 100644 include/image-fit.h

diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
index 7bb9b436560c..22c2d6017e7c 100644
--- a/arch/arm/lib/bootm.c
+++ b/arch/arm/lib/bootm.c
@@ -553,6 +553,78 @@ BAREBOX_MAGICVAR(aimage_noverwrite_bootargs, "Disable 
overwrite of the bootargs
 BAREBOX_MAGICVAR(aimage_noverwrite_tags, "Disable overwrite of the tags addr 
with the one present in aimage");
 #endif
 
+#include 
+
+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(_start, _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);
+
+   return __do_bootm_linux(data, mem_free, 0);
+
+err_out:
+   if (handle)
+   fit_close(handle);
+   return ret;
+}
+
+static struct image_handler arm_fit_handler = {
+.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(_handler);
binfmt_register(_aimage_hook);
}
+   if (IS_BUILTIN(CONFIG_CMD_BOOTM_FITIMAGE))
+   register_image_handler(_fit_handler);
binfmt_register(_arm_zimage_hook);
binfmt_register(_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.
+
 config CMD_BOOTU
tristate
default y
diff --git a/common/Kconfig b/common/Kconfig
index 8e7950968c3e..d824b5e35f04 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -66,6 +66,12 @@ config UIMAGE
select CRC32
bool
 
+config FITIMAGE
+   bool
+   select OFTREE
+   select DIGEST
+   select CRYPTO_RSA
+
 config LOGBUF
bool
 
diff --git 

Re: [PATCH 3/3] bootm: add initial FIT support

[Yegor Yefremov]

Hi Marc,

thanks for reposting the patches.

On Tue, Jan 5, 2016 at 9:11 AM, Marc Kleine-Budde  wrote:
> From: Jan Luebbe 
>
> This implementation is inspired by U-Boot's FIT support. Instead of
> using libfdt (which does not exist in barebox), configuration signatures
> are verified by using a simplified DT parser based on barebox's own
> code.
>
> Currently, only signed configurations with hashed images are supported,
> as the other variants are less useful for verified boot. Compatible FIT
> images can be created using U-Boot's mkimage tool.

What about unsigned images?

I also get: unsupported algo crc32

Is it intended to be supported?

> Signed-off-by: Jan Luebbe 
> Signed-off-by: Marc Kleine-Budde 
> ---
>  arch/arm/lib/bootm.c |  74 +++
>  commands/Kconfig |   8 +
>  common/Kconfig   |   6 +
>  common/Makefile  |   1 +
>  common/image-fit.c   | 590 
> +++
>  include/image-fit.h  |  42 
>  6 files changed, 721 insertions(+)
>  create mode 100644 common/image-fit.c
>  create mode 100644 include/image-fit.h
>
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 7bb9b436560c..22c2d6017e7c 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -553,6 +553,78 @@ BAREBOX_MAGICVAR(aimage_noverwrite_bootargs, "Disable 
> overwrite of the bootargs
>  BAREBOX_MAGICVAR(aimage_noverwrite_tags, "Disable overwrite of the tags addr 
> with the one present in aimage");
>  #endif
>
> +#include 
> +
> +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(_start, _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);
> +
> +   return __do_bootm_linux(data, mem_free, 0);
> +
> +err_out:
> +   if (handle)
> +   fit_close(handle);
> +   return ret;
> +}
> +
> +static struct image_handler arm_fit_handler = {
> +.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(_handler);
> binfmt_register(_aimage_hook);
> }
> +   if (IS_BUILTIN(CONFIG_CMD_BOOTM_FITIMAGE))
> +   register_image_handler(_fit_handler);
> binfmt_register(_arm_zimage_hook);
> binfmt_register(_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

Re: [PATCH 3/3] bootm: add initial FIT support

[Marc Kleine-Budde]

On 01/05/2016 11:28 AM, Yegor Yefremov wrote:
> Hi Marc,
> 
> thanks for reposting the patches.
> 
> On Tue, Jan 5, 2016 at 9:11 AM, Marc Kleine-Budde  wrote:
>> From: Jan Luebbe 
>>
>> This implementation is inspired by U-Boot's FIT support. Instead of
>> using libfdt (which does not exist in barebox), configuration signatures
>> are verified by using a simplified DT parser based on barebox's own
>> code.
>>
>> Currently, only signed configurations with hashed images are supported,
>> as the other variants are less useful for verified boot. Compatible FIT
>> images can be created using U-Boot's mkimage tool.
> 
> What about unsigned images?

That's not our use case. We use plain zImages instead.

> I also get: unsupported algo crc32
> Is it intended to be supported?

Not for our usecase - feel free to add crc32 support.

Marc

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



signature.asc
Description: OpenPGP digital signature
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Marc Kleine-Budde]

On 01/05/2016 02:05 PM, Yegor Yefremov wrote:
> What about unsigned images?

 That's not our use case. We use plain zImages instead.
>>>
>>> The solution would be to introduce an option like in U-Boot?
>>>
>>> CONFIG_FIT_SIGNATURE:
>>>
>>> This option enables signature verification of FIT uImages,
>>> using a hash signed and verified using RSA. If
>>> CONFIG_SHA_PROG_HW_ACCEL is defined, i.e support for progressive
>>> hashing is available using hardware, RSA library will use it.
>>> See doc/uImage.FIT/signature.txt for more details.
>>
>> Technically possible, but I'm not sure what are the benefits of using
>> fit images, if you don't need signatures. barebox implements
>> freedesktop.org's bootspec and this is IMHO the way to go.
> 
> For me FIT is just a way to have a kernel and a bunch of device tree
> blobs in one file. Signed or not signed is an option for me. Just like
> U-Boot implements it. This is user responsibility.

Send patches. :D

> In my use case I just read device ID from EEPROM, load my
> kernel-fit.itb and select needed DTB via this ID. This way I have only
> one SD card image, that can be run on more, than 10 different devices
> using the same core module.

> I also get: unsupported algo crc32
> Is it intended to be supported?

 Not for our usecase - feel free to add crc32 support.
>>>
>>> OK.
>>>
>>> But what about FIT configuration selection syntax?
>>
>> What's this?
> 
> Have you seen my comments to this patch regarding
> fit_open_configuration() routine?

sorry - I've missed that. Too many quoted lines. :D

>> > +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);
> 
> This is not working for my *.its file:
> https://github.com/visionsystemsgmbh/onrisc_br_bsp/blob/master/board/vscom/baltos/kernel-fit.its
> U-Boot is working with bootm ${loadaddr}#conf${board_name}
> 
> For Barebox I've changed this line to
> 
> snprintf(unit_name, sizeof(unit_name), "conf%d@1", num)
> 
> This is how I start Linux: bootm /boot/kernel-fit.itb@$global.board.id
> 
> What is the standard for providing FIT configuration?

Don't know. Is there a spec in the u-boot sources, otherwise use the code.

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



signature.asc
Description: OpenPGP digital signature
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Yegor Yefremov]

On Tue, Jan 5, 2016 at 2:50 PM, Marc Kleine-Budde  wrote:
> On 01/05/2016 02:05 PM, Yegor Yefremov wrote:
>> What about unsigned images?
>
> That's not our use case. We use plain zImages instead.

 The solution would be to introduce an option like in U-Boot?

 CONFIG_FIT_SIGNATURE:

 This option enables signature verification of FIT uImages,
 using a hash signed and verified using RSA. If
 CONFIG_SHA_PROG_HW_ACCEL is defined, i.e support for progressive
 hashing is available using hardware, RSA library will use it.
 See doc/uImage.FIT/signature.txt for more details.
>>>
>>> Technically possible, but I'm not sure what are the benefits of using
>>> fit images, if you don't need signatures. barebox implements
>>> freedesktop.org's bootspec and this is IMHO the way to go.
>>
>> For me FIT is just a way to have a kernel and a bunch of device tree
>> blobs in one file. Signed or not signed is an option for me. Just like
>> U-Boot implements it. This is user responsibility.
>
> Send patches. :D

I'll prepare one on top of yours. So far I can boot into Linux on my
am335x based board, so

Tested-by: Yegor Yefremov 

>> In my use case I just read device ID from EEPROM, load my
>> kernel-fit.itb and select needed DTB via this ID. This way I have only
>> one SD card image, that can be run on more, than 10 different devices
>> using the same core module.
>
>> I also get: unsupported algo crc32
>> Is it intended to be supported?
>
> Not for our usecase - feel free to add crc32 support.

 OK.

 But what about FIT configuration selection syntax?
>>>
>>> What's this?
>>
>> Have you seen my comments to this patch regarding
>> fit_open_configuration() routine?
>
> sorry - I've missed that. Too many quoted lines. :D
>
>>> > +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);
>>
>> This is not working for my *.its file:
>> https://github.com/visionsystemsgmbh/onrisc_br_bsp/blob/master/board/vscom/baltos/kernel-fit.its
>> U-Boot is working with bootm ${loadaddr}#conf${board_name}
>>
>> For Barebox I've changed this line to
>>
>> snprintf(unit_name, sizeof(unit_name), "conf%d@1", num)
>>
>> This is how I start Linux: bootm /boot/kernel-fit.itb@$global.board.id
>>
>> What is the standard for providing FIT configuration?
>
> Don't know. Is there a spec in the u-boot sources, otherwise use the code.

Will look closer at U-Boot code and send a patch.

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Marc Kleine-Budde]

On 01/05/2016 11:40 AM, Yegor Yefremov wrote:
> On Tue, Jan 5, 2016 at 11:32 AM, Marc Kleine-Budde  
> wrote:
>> On 01/05/2016 11:28 AM, Yegor Yefremov wrote:
>>> Hi Marc,
>>>
>>> thanks for reposting the patches.
>>>
>>> On Tue, Jan 5, 2016 at 9:11 AM, Marc Kleine-Budde  
>>> wrote:
 From: Jan Luebbe 

 This implementation is inspired by U-Boot's FIT support. Instead of
 using libfdt (which does not exist in barebox), configuration signatures
 are verified by using a simplified DT parser based on barebox's own
 code.

 Currently, only signed configurations with hashed images are supported,
 as the other variants are less useful for verified boot. Compatible FIT
 images can be created using U-Boot's mkimage tool.
>>>
>>> What about unsigned images?
>>
>> That's not our use case. We use plain zImages instead.
> 
> The solution would be to introduce an option like in U-Boot?
> 
> CONFIG_FIT_SIGNATURE:
> 
> This option enables signature verification of FIT uImages,
> using a hash signed and verified using RSA. If
> CONFIG_SHA_PROG_HW_ACCEL is defined, i.e support for progressive
> hashing is available using hardware, RSA library will use it.
> See doc/uImage.FIT/signature.txt for more details.

Technically possible, but I'm not sure what are the benefits of using
fit images, if you don't need signatures. barebox implements
freedesktop.org's bootspec and this is IMHO the way to go.

>>> I also get: unsupported algo crc32
>>> Is it intended to be supported?
>>
>> Not for our usecase - feel free to add crc32 support.
> 
> OK.
> 
> But what about FIT configuration selection syntax?

What's this?

Marc

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



signature.asc
Description: OpenPGP digital signature
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Yegor Yefremov]

On Tue, Jan 5, 2016 at 11:32 AM, Marc Kleine-Budde  wrote:
> On 01/05/2016 11:28 AM, Yegor Yefremov wrote:
>> Hi Marc,
>>
>> thanks for reposting the patches.
>>
>> On Tue, Jan 5, 2016 at 9:11 AM, Marc Kleine-Budde  
>> wrote:
>>> From: Jan Luebbe 
>>>
>>> This implementation is inspired by U-Boot's FIT support. Instead of
>>> using libfdt (which does not exist in barebox), configuration signatures
>>> are verified by using a simplified DT parser based on barebox's own
>>> code.
>>>
>>> Currently, only signed configurations with hashed images are supported,
>>> as the other variants are less useful for verified boot. Compatible FIT
>>> images can be created using U-Boot's mkimage tool.
>>
>> What about unsigned images?
>
> That's not our use case. We use plain zImages instead.

The solution would be to introduce an option like in U-Boot?

CONFIG_FIT_SIGNATURE:

This option enables signature verification of FIT uImages,
using a hash signed and verified using RSA. If
CONFIG_SHA_PROG_HW_ACCEL is defined, i.e support for progressive
hashing is available using hardware, RSA library will use it.
See doc/uImage.FIT/signature.txt for more details.

>> I also get: unsupported algo crc32
>> Is it intended to be supported?
>
> Not for our usecase - feel free to add crc32 support.

OK.

But what about FIT configuration selection syntax?

Yegor

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Yegor Yefremov]

On Tue, Jan 5, 2016 at 12:54 PM, Marc Kleine-Budde  wrote:
> On 01/05/2016 11:40 AM, Yegor Yefremov wrote:
>> On Tue, Jan 5, 2016 at 11:32 AM, Marc Kleine-Budde  
>> wrote:
>>> On 01/05/2016 11:28 AM, Yegor Yefremov wrote:
 Hi Marc,

 thanks for reposting the patches.

 On Tue, Jan 5, 2016 at 9:11 AM, Marc Kleine-Budde  
 wrote:
> From: Jan Luebbe 
>
> This implementation is inspired by U-Boot's FIT support. Instead of
> using libfdt (which does not exist in barebox), configuration signatures
> are verified by using a simplified DT parser based on barebox's own
> code.
>
> Currently, only signed configurations with hashed images are supported,
> as the other variants are less useful for verified boot. Compatible FIT
> images can be created using U-Boot's mkimage tool.

 What about unsigned images?
>>>
>>> That's not our use case. We use plain zImages instead.
>>
>> The solution would be to introduce an option like in U-Boot?
>>
>> CONFIG_FIT_SIGNATURE:
>>
>> This option enables signature verification of FIT uImages,
>> using a hash signed and verified using RSA. If
>> CONFIG_SHA_PROG_HW_ACCEL is defined, i.e support for progressive
>> hashing is available using hardware, RSA library will use it.
>> See doc/uImage.FIT/signature.txt for more details.
>
> Technically possible, but I'm not sure what are the benefits of using
> fit images, if you don't need signatures. barebox implements
> freedesktop.org's bootspec and this is IMHO the way to go.

For me FIT is just a way to have a kernel and a bunch of device tree
blobs in one file. Signed or not signed is an option for me. Just like
U-Boot implements it. This is user responsibility.

In my use case I just read device ID from EEPROM, load my
kernel-fit.itb and select needed DTB via this ID. This way I have only
one SD card image, that can be run on more, than 10 different devices
using the same core module.

 I also get: unsupported algo crc32
 Is it intended to be supported?
>>>
>>> Not for our usecase - feel free to add crc32 support.
>>
>> OK.
>>
>> But what about FIT configuration selection syntax?
>
> What's this?

Have you seen my comments to this patch regarding
fit_open_configuration() routine?

http://lists.infradead.org/pipermail/barebox/2016-January/025718.html

Yegor

___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] bootm: add initial FIT support

[Trent Piepho]

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(_start, _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?
> +
> + 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.

> + return ret;
> +}
> +
> +static struct image_handler arm_fit_handler = {

Can this be const?

> +.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(_handler);
>   binfmt_register(_aimage_hook);
>   }
> + if (IS_BUILTIN(CONFIG_CMD_BOOTM_FITIMAGE))
> + register_image_handler(_fit_handler);
>   binfmt_register(_arm_zimage_hook);
>   binfmt_register(_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?


> +/*
> + * 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", _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", _len);
> + if (!sig_value)