[v2,1/3] handlers: add readback handler

From: Kevin Zhang <kevin.zhang3@cn.bosch.com>

To verify that an image was written properly, this readback handler
calculates the sha256 hash of a partition (or part of it) and compares
it against a given hash value.

Signed-off-by: Kevin Zhang <kevin.zhang3@cn.bosch.com>
Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
---
 handlers/Config.in          |  14 +++-
 handlers/Makefile           |   1 +
 handlers/readback_handler.c | 124 ++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 handlers/readback_handler.c

Hi Mark,

On 20/01/20 13:43, 'Mark Jonas' via swupdate wrote:
> From: Kevin Zhang <kevin.zhang3@cn.bosch.com>
> 
> To verify that an image was written properly, this readback handler
> calculates the sha256 hash of a partition (or part of it) and compares
> it against a given hash value.
> 
> Signed-off-by: Kevin Zhang <kevin.zhang3@cn.bosch.com>
> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com>
> ---
>  handlers/Config.in          |  14 +++-
>  handlers/Makefile           |   1 +
>  handlers/readback_handler.c | 124 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+), 1 deletion(-)
>  create mode 100644 handlers/readback_handler.c
> 
> diff --git a/handlers/Config.in b/handlers/Config.in
> index 41eac1c..be1c7a0 100644
> --- a/handlers/Config.in
> +++ b/handlers/Config.in
> @@ -48,7 +48,7 @@ config UBIWHITELIST
>  	help
>  	  Define a list of MTD devices that are planned to have
>  	  always UBI. If first attach fails, the device is erased
> -	  and tried again. 
> +	  and tried again.

This is annoying. It has nothing to do here - I do not see the issue,
and if there is one, it should be handled in a separate patch.

>  	  The list can be set as a string with the mtd numbers.
>  	  Examples: "0 1 2"
>  	  This sets mtd0-mtd1-mtd2 to be used as UBI volumes.
> @@ -106,6 +106,18 @@ config RDIFFHANDLER
>  	  Add support for applying librsync's rdiff patches,
>  	  see http://librsync.sourcefrog.net/
>  
> +config READBACKHANDLER
> +	bool "readback"
> +	depends on HASH_VERIFY
> +	default n
> +	help
> +	  To verify that an image was written properly, this readback handler
> +	  calculates the sha256 hash of a partition (or part of it) and compares
> +	  it against a given hash value.
> +
> +	  This is a post-install handler running at the same time as
> +	  post-install scripts.
> +
>  config LUASCRIPTHANDLER
>  	bool "Lua Script"
>  	depends on LUA
> diff --git a/handlers/Makefile b/handlers/Makefile
> index 61e4f76..b756f31 100644
> --- a/handlers/Makefile
> +++ b/handlers/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_CFIHAMMING1)+= flash_hamming1_handler.o
>  obj-$(CONFIG_LUASCRIPTHANDLER) += lua_scripthandler.o
>  obj-$(CONFIG_RAW)	+= raw_handler.o
>  obj-$(CONFIG_RDIFFHANDLER) += rdiff_handler.o
> +obj-$(CONFIG_READBACKHANDLER) += readback_handler.o
>  obj-$(CONFIG_REMOTE_HANDLER) += remote_handler.o
>  obj-$(CONFIG_SHELLSCRIPTHANDLER) += shell_scripthandler.o
>  obj-$(CONFIG_SSBLSWITCH) += ssbl_handler.o
> diff --git a/handlers/readback_handler.c b/handlers/readback_handler.c
> new file mode 100644
> index 0000000..d99a574
> --- /dev/null
> +++ b/handlers/readback_handler.c
> @@ -0,0 +1,124 @@
> +/*
> + * SPDX-FileCopyrightText: 2020 Bosch Sicherheitssysteme GmbH
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <sys/ioctl.h>
> +#include <linux/fs.h>
> +
> +#include "swupdate.h"
> +#include "handler.h"
> +#include "sslapi.h"
> +#include "util.h"
> +
> +void readback_handler(void);
> +static int readback_postinst(struct img_type *img);
> +
> +static int readback(struct img_type *img, void *data)
> +{
> +	if (!data)
> +		return -1;
> +
> +	script_fn scriptfn = *(script_fn *)data;
> +	switch (scriptfn) {
> +	case POSTINSTALL:
> +		return readback_postinst(img);
> +	case PREINSTALL:
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int readback_postinst(struct img_type *img)
> +{
> +	/* Get property: partition hash */
> +	unsigned char hash[SHA256_HASH_LENGTH];
> +	char *ascii_hash = dict_get_value(&img->properties, "sha256");
> +	if (!ascii_hash || ascii_to_hash(hash, ascii_hash) < 0 || !IsValidHash(hash)) {
> +		ERROR("Invalid hash");
> +		return -EINVAL;
> +	}
> +
> +	/* Get property: partition size */
> +	unsigned int size = 0;
> +	char *value = dict_get_value(&img->properties, "size");
> +	if (value) {
> +		size = strtoul(value, NULL, 10);
> +	} else {
> +		TRACE("Property size not found, use partition size");
> +	}
> +
> +	/* Get property: offset */
> +	unsigned long offset = 0;
> +	value = dict_get_value(&img->properties, "offset");
> +	if (value) {
> +		offset = strtoul(value, NULL, 10);
> +	} else {
> +		TRACE("Property offset not found, use default 0");
> +	}
> +
> +	/* Open the device (partition) */
> +	int fdin = open(img->device, O_RDONLY);
> +	if (fdin < 0) {
> +		ERROR("Failed to open %s: %s", img->device, strerror(errno));

But if the device to be checked cannot be opened, the handler cannot go
on. In case of error, handler should stop and return the error to the
caller.

> +	}
> +
> +	/* Get the real size of the partition, if size is not set. */
> +	if (size == 0) {
> +		if (ioctl(fdin, BLKGETSIZE64, &size) < 0) {
> +			ERROR("Cannot get size of %s", img->device);
> +			close(fdin);
> +			return -EFAULT;
> +		}
> +		TRACE("Partition size: %u", size);
> +	}
> +
> +	/* 
> +	 * Seek the file descriptor before passing it to copyfile().
> +	 * This is necessary because copyfile() only accepts streams,
> +	 * so the file descriptor shall be already at the right position.
> +	 */
> +	if (lseek(fdin, offset, SEEK_SET) < 0) {
> +		ERROR("Seek %lu bytes failed: %s", offset, strerror(errno));
> +		close(fdin);
> +		return -EFAULT;
> +	}
> +
> +	/*
> +	 * Perform hash verification. We do not need to pass an output device to
> +	 * the copyfile() function, because we only want it to verify the hash of
> +	 * the input device.
> +	 */
> +	unsigned long offset_out = 0;
> +	int status = copyfile(fdin,
> +			NULL,  /* no output */
> +			size,
> +			&offset_out,
> +			0,     /* no output seek */
> +			1,     /* skip file, do not write to the output */
> +			0,     /* no compressed */
> +			NULL,  /* no checksum */
> +			hash,
> +			0,     /* no encrypted */
> +			NULL); /* no callback */

Do you tested this ? It does not look correct.

 int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long
*offs, unsigned long long seek,
         int skip_file, int __attribute__ ((__unused__)) compressed,
         uint32_t *checksum, unsigned char *hash, int encrypted,
writeimage callback)

To avoid to write the output, you pass NULL for "out" and "callback".
Because callback is NULL, the standard callback "copy_write" is called.
But if out == NULL, fd is set to -1 and it should returns with an error.
Does it works on your side ?

The code above should work if you pass a dummy calback, somethin like

int dummy_write(void *out, const void *buf, unsigned int len) {
	return 0;
}

If not, I am expecting an error from copyfile()

> +	if (status == 0) {
> +		INFO("Readback verification success");
> +	} else {
> +		ERROR("Readback verification failed, status=%d", status);
> +	}
> +
> +	close(fdin);
> +	return status;
> +}
> +
> +__attribute__((constructor))
> +void readback_handler(void)
> +{
> +	register_handler("readback", readback, SCRIPT_HANDLER | NO_DATA_HANDLER, NULL);
> +}
> 

Best regards,
Stefano Babic

Hi Stefano,

> > diff --git a/handlers/Config.in b/handlers/Config.in index
> > 41eac1c..be1c7a0 100644
> > --- a/handlers/Config.in
> > +++ b/handlers/Config.in
> > @@ -48,7 +48,7 @@ config UBIWHITELIST
> >  	help
> >  	  Define a list of MTD devices that are planned to have
> >  	  always UBI. If first attach fails, the device is erased
> > -	  and tried again.
> > +	  and tried again.
> 
> This is annoying. It has nothing to do here - I do not see the issue, and if
> there is one, it should be handled in a separate patch.

It is annoying and I apologize. I messed up here. There is a trailing
white space. We will add it again in this patch. There will be an
additional patch at the end of the series to remove it.

> > --- /dev/null
> > +++ b/handlers/readback_handler.c
[..]
> > +	/* Open the device (partition) */
> > +	int fdin = open(img->device, O_RDONLY);
> > +	if (fdin < 0) {
> > +		ERROR("Failed to open %s: %s", img->device,
> strerror(errno));
> 
> But if the device to be checked cannot be opened, the handler cannot go
> on. In case of error, handler should stop and return the error to the caller.

Right, we will fix this.

> > +	/*
> > +	 * Perform hash verification. We do not need to pass an output
> device to
> > +	 * the copyfile() function, because we only want it to verify the
> hash of
> > +	 * the input device.
> > +	 */
> > +	unsigned long offset_out = 0;
> > +	int status = copyfile(fdin,
> > +			NULL,  /* no output */
> > +			size,
> > +			&offset_out,
> > +			0,     /* no output seek */
> > +			1,     /* skip file, do not write to the output */
> > +			0,     /* no compressed */
> > +			NULL,  /* no checksum */
> > +			hash,
> > +			0,     /* no encrypted */
> > +			NULL); /* no callback */
> 
> Do you tested this ? It does not look correct.
> 
>  int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs,
> unsigned long long seek,
>          int skip_file, int __attribute__ ((__unused__)) compressed,
>          uint32_t *checksum, unsigned char *hash, int encrypted, writeimage
> callback)
> 
> To avoid to write the output, you pass NULL for "out" and "callback".
> Because callback is NULL, the standard callback "copy_write" is called.
> But if out == NULL, fd is set to -1 and it should returns with an error.
> Does it works on your side ?
> 
> The code above should work if you pass a dummy calback, somethin like
> 
> int dummy_write(void *out, const void *buf, unsigned int len) {
> 	return 0;
> }
> 
> If not, I am expecting an error from copyfile()

We tested the code and it works: 

 - Because seek is 0, there will be no seeking of out. Thus out is not
   used in line 465ff.

 - If skip_file is true the callback copy_write won't be called and out
   won't be used. See line 511.

Greetings,
Mark

Mark Jonas

Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Christian Fischer; Geschäftsführung: Tanja Rückert, Andreas Bartz, Thomas Quante

Hi Mark,

On 20/01/20 16:19, Jonas Mark (BT-FIR/ENG1-Grb) wrote:
> Hi Stefano,
> 
>>> diff --git a/handlers/Config.in b/handlers/Config.in index
>>> 41eac1c..be1c7a0 100644
>>> --- a/handlers/Config.in
>>> +++ b/handlers/Config.in
>>> @@ -48,7 +48,7 @@ config UBIWHITELIST
>>>  	help
>>>  	  Define a list of MTD devices that are planned to have
>>>  	  always UBI. If first attach fails, the device is erased
>>> -	  and tried again.
>>> +	  and tried again.
>>
>> This is annoying. It has nothing to do here - I do not see the issue, and if
>> there is one, it should be handled in a separate patch.
> 
> It is annoying and I apologize. I messed up here. There is a trailing
> white space. We will add it again in this patch. There will be an
> additional patch at the end of the series to remove it.
> 
>>> --- /dev/null
>>> +++ b/handlers/readback_handler.c
> [..]
>>> +	/* Open the device (partition) */
>>> +	int fdin = open(img->device, O_RDONLY);
>>> +	if (fdin < 0) {
>>> +		ERROR("Failed to open %s: %s", img->device,
>> strerror(errno));
>>
>> But if the device to be checked cannot be opened, the handler cannot go
>> on. In case of error, handler should stop and return the error to the caller.
> 
> Right, we will fix this.
> 

ok

>>> +	/*
>>> +	 * Perform hash verification. We do not need to pass an output
>> device to
>>> +	 * the copyfile() function, because we only want it to verify the
>> hash of
>>> +	 * the input device.
>>> +	 */
>>> +	unsigned long offset_out = 0;
>>> +	int status = copyfile(fdin,
>>> +			NULL,  /* no output */
>>> +			size,
>>> +			&offset_out,
>>> +			0,     /* no output seek */
>>> +			1,     /* skip file, do not write to the output */
>>> +			0,     /* no compressed */
>>> +			NULL,  /* no checksum */
>>> +			hash,
>>> +			0,     /* no encrypted */
>>> +			NULL); /* no callback */
>>
>> Do you tested this ? It does not look correct.
>>
>>  int copyfile(int fdin, void *out, unsigned int nbytes, unsigned long *offs,
>> unsigned long long seek,
>>          int skip_file, int __attribute__ ((__unused__)) compressed,
>>          uint32_t *checksum, unsigned char *hash, int encrypted, writeimage
>> callback)
>>
>> To avoid to write the output, you pass NULL for "out" and "callback".
>> Because callback is NULL, the standard callback "copy_write" is called.
>> But if out == NULL, fd is set to -1 and it should returns with an error.
>> Does it works on your side ?
>>
>> The code above should work if you pass a dummy calback, somethin like
>>
>> int dummy_write(void *out, const void *buf, unsigned int len) {
>> 	return 0;
>> }
>>
>> If not, I am expecting an error from copyfile()
> 
> We tested the code and it works: 
> 
>  - Because seek is 0, there will be no seeking of out. Thus out is not
>    used in line 465ff.

Agree.

> 
>  - If skip_file is true the callback copy_write won't be called and out
>    won't be used. See line 511.

You're definetely right: the trick is done here by skip_file, your code
is correct. If you just send a V3 with the small nitpicks above, I'll
merge it soon.

Best regards,
Stefano