Re: [PATCH v3 2/3] imd: add support for checksum generation/verification

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jan 20, 2020 at 03:58:20PM +0100, Steffen Trumtrar wrote:
> Add a new imd type "checksum". This type consists of the CRC32 checksum
> of the whole barebox image minus the checksum itself.
> The checksum can be written to the imd field with the bareboximd host-tool.
> It can be verified with said tool or with "imd" on the target.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
> Changes in v3:
>   - moved the flags field to the uint32_t tag
> 
>  commands/imd.c           |   1 +
>  common/imd-barebox.c     |   1 +
>  common/imd.c             | 169 ++++++++++++++++++++++++++++++++++++++-
>  include/image-metadata.h |  42 ++++++++++
>  scripts/bareboximd.c     |  32 ++++++++
>  5 files changed, 244 insertions(+), 1 deletion(-)
> 
> diff --git a/commands/imd.c b/commands/imd.c
> index f1a22cef96bd..16ab7290c920 100644
> --- a/commands/imd.c
> +++ b/commands/imd.c
> @@ -46,6 +46,7 @@ BAREBOX_CMD_HELP_TEXT("Options:")
>  BAREBOX_CMD_HELP_OPT ("-t <type>", "only show information of <type>")
>  BAREBOX_CMD_HELP_OPT ("-n <no>", "for tags with multiple strings only show string <no>")
>  BAREBOX_CMD_HELP_OPT ("-s VARNAME",  "set variable VARNAME instead of showing information")
> +BAREBOX_CMD_HELP_OPT ("-V",  "Verify checksum of image")
>  BAREBOX_CMD_HELP_TEXT("")
>  BAREBOX_CMD_HELP_TEXT("Without options all information available is printed. Valid types are:")
>  BAREBOX_CMD_HELP_TEXT("release, build, model, of_compatible")
> diff --git a/common/imd-barebox.c b/common/imd-barebox.c
> index e9cd37d83ec8..4aec51bfbdc2 100644
> --- a/common/imd-barebox.c
> +++ b/common/imd-barebox.c
> @@ -23,3 +23,4 @@ __BAREBOX_IMD_SECTION(.barebox_imd_end) = {
>  
>  BAREBOX_IMD_TAG_STRING(imd_build_tag, IMD_TYPE_BUILD, UTS_VERSION, 1);
>  BAREBOX_IMD_TAG_STRING(imd_release_tag, IMD_TYPE_RELEASE, UTS_RELEASE, 1);
> +BAREBOX_IMD_CRC(imd_checksum, 0x0, 1);
> diff --git a/common/imd.c b/common/imd.c
> index e0dab69644c0..b8429184777b 100644
> --- a/common/imd.c
> +++ b/common/imd.c
> @@ -22,6 +22,7 @@
>  #include <getopt.h>
>  #include <malloc.h>
>  #include <fs.h>
> +#include <crc.h>
>  
>  #ifndef CONFIG_CMD_IMD
>  int imd_command_setenv(const char *variable_name, const char *value)
> @@ -167,6 +168,9 @@ static struct imd_type_names imd_types[] = {
>  	}, {
>  		.type = IMD_TYPE_OF_COMPATIBLE,
>  		.name = "of_compatible",
> +	}, {
> +		.type = IMD_TYPE_CHECKSUM,
> +		.name = "checksum",

Given that it's specifically a CRC32 sum it should be named like this.
In the end a sh256sum is also a checksum, so we shouldn't pollute the
namespace like this.

> @@ -299,10 +449,12 @@ int imd_command(int argc, char *argv[])
>  	const char *filename;
>  	const char *variable_name = NULL;
>  	char *str;
> +	uint32_t checksum = 0;
> +	uint32_t verify = 0;
>  
>  	imd_command_verbose = 0;
>  
> -	while ((opt = getopt(argc, argv, "vt:s:n:")) > 0) {
> +	while ((opt = getopt(argc, argv, "vt:s:n:cV")) > 0) {
>  		switch(opt) {
>  		case 't':
>  			type = imd_name_to_type(optarg);
> @@ -320,6 +472,12 @@ int imd_command(int argc, char *argv[])
>  		case 'n':
>  			strno = simple_strtoul(optarg, NULL, 0);
>  			break;
> +		case 'c':
> +			checksum = 1;
> +			break;
> +		case 'V':
> +			verify = 1;
> +			break;

These new options are not documented in commands/imd.c.

The Linux imd command should have a help function as well, but that's a
different topic.

> diff --git a/include/image-metadata.h b/include/image-metadata.h
> index 5904d95acd37..637a611a39a1 100644
> --- a/include/image-metadata.h
> +++ b/include/image-metadata.h
> @@ -25,9 +25,12 @@
>  #define IMD_TYPE_MODEL		0x640c8004 /* The board name this image is for */
>  #define IMD_TYPE_OF_COMPATIBLE	0x640c8005 /* the device tree compatible string */
>  #define IMD_TYPE_PARAMETER	0x640c8006 /* A generic parameter. Use key=value as data */
> +#define IMD_TYPE_CHECKSUM	0x640c1007 /* the checksum of the barebox images */
>  #define IMD_TYPE_END		0x640c7fff
>  #define IMD_TYPE_INVALID	0xffffffff
>  
> +#define IMD_FLAG_TAG_VALID	(1 << 0)

This should be specific to a crc32 tag.

> +
>  /*
>   * The IMD header. All data is stored in little endian format in the image.
>   * The next header starts at the next 4 byte boundary after the data.
> @@ -51,6 +54,25 @@ static inline int imd_is_string(uint32_t type)
>  	return (type & 0x8000) ? 1 : 0;
>  }
>  
> +static inline int imd_is_uint32(uint32_t type)
> +{
> +	return (type & 0x1000) ? 1 : 0;
> +}

Erm, no. Please don't reserve bits in the IMD_TYPE_ defines without
documenting it anywhere.

> +
> +static inline int imd_entry_is_valid(uint32_t flags)
> +{
> +	return (flags & IMD_FLAG_TAG_VALID) ? 1 : 0;
> +}
> +
> +/*
> + * A IMD int.
> + */
> +struct imd_entry_uint32 {
> +	struct imd_header header;
> +	uint32_t data;
> +	uint32_t flags;
> +};

Given that this is not a plain uint32_t but has an additional flags
field this should really be a struct imd_entry_crc32.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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