Re: [PATCH 2/2] images: add function to verify checksum in barebox header

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

On Mon, May 23, 2016 at 01:48:42PM +0200, Ulrich Ölmann wrote:
> From: Jan Luebbe <jlu@pengutronix.de>
> 
> Signed-off-by: Jan Luebbe <jlu@pengutronix.de>
> Signed-off-by: Ulrich Ölmann <u.oelmann@pengutronix.de>
> ---
>  common/Kconfig         |  9 ++++++
>  common/Makefile        |  1 +
>  common/barebox-image.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/common.h       |  9 ++++++
>  4 files changed, 104 insertions(+)
>  create mode 100644 common/barebox-image.c
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index 928db0a159e1..bebbb614509d 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -580,6 +580,15 @@ config IMD_TARGET
>  	depends on IMD
>  	depends on !SANDBOX
>  
> +config VERIFY_EMBEDDED_CRC
> +	prompt "Verify the CRC checksum embedded into barebox images"
> +	bool
> +	select CRC32
> +	help
> +	  If you say Y here, the CRC checksum that is embedded into every
> +	  barebox image directly after the header can be verified to guarantee
> +	  the integrity of the image.

When a user reads this he probably expects the option to have an effect.
In fact this option only adds a currently unused function. You should
make it invisible.

> +
>  config KERNEL_INSTALL_TARGET
>  	bool
>  	depends on !SANDBOX
> diff --git a/common/Makefile b/common/Makefile
> index 99681e21215b..0106f0fe7bd9 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_STATE)		+= state.o
>  obj-$(CONFIG_RATP)		+= ratp.o
>  obj-$(CONFIG_UIMAGE)		+= image.o uimage.o
>  obj-$(CONFIG_FITIMAGE)		+= image-fit.o
> +obj-$(CONFIG_VERIFY_EMBEDDED_CRC) += barebox-image.o
>  obj-$(CONFIG_MENUTREE)		+= menutree.o
>  obj-$(CONFIG_EFI_GUID)		+= efi-guid.o
>  obj-$(CONFIG_EFI_DEVICEPATH)	+= efi-devicepath.o
> diff --git a/common/barebox-image.c b/common/barebox-image.c
> new file mode 100644
> index 000000000000..733d9ea2ecdf
> --- /dev/null
> +++ b/common/barebox-image.c
> @@ -0,0 +1,85 @@
> +/*
> + * Copyright (c) 2016 Pengutronix, Jan Luebbe <j.luebbe@pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +#include <common.h>
> +#include <io.h>
> +#include <filetype.h>
> +
> +static inline size_t get_barebox_head_size(const void *buf)
> +{
> +	if (is_barebox_arm_head(buf))
> +		return ARM_HEAD_SIZE;
> +	else if (is_barebox_mips_head(buf))
> +		return MIPS_HEAD_SIZE;
> +
> +	return 0;
> +}
> +
> +static inline size_t get_barebox_head_size_offset(const void *buf)
> +{
> +	if (is_barebox_arm_head(buf))
> +		return ARM_HEAD_SIZE_OFFSET;
> +	else if (is_barebox_mips_head(buf))
> +		return MIPS_HEAD_SIZE_OFFSET;
> +
> +	return 0;
> +}
> +
> +bool barebox_verify_image(void *buf, size_t bufsize)

Since this code is called from architecture specific code anyway it's
probably nicer to create [arm|mips]_barebox_verify functions. This would
allow you to have only one meaningful size check.

> +{
> +	unsigned int *psize;
> +	size_t head_size;
> +	uint32_t crc = 0;
> +	uint32_t *p;
> +
> +	if (bufsize < ARM_HEAD_SIZE) {
> +		pr_err("buffer is smaller than ARM_HEAD_SIZE\n");
> +		return false;
> +	}

I think the error messaging should be left to the caller. The caller can
provide more context to make the message more useful. For this it might
be useful to return int instead of bool from this function as it allows
to return error codes.

Sascha

-- 
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-5555 |

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