[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          |   7 ++
 handlers/Makefile           |   1 +
 handlers/readback_handler.c | 161 ++++++++++++++++++++++++++++++++++++
 3 files changed, 169 insertions(+)
 create mode 100644 handlers/readback_handler.c

Hi Mark, Kevin,

On 16/01/20 17:12, '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          |   7 ++
>  handlers/Makefile           |   1 +
>  handlers/readback_handler.c | 161 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 169 insertions(+)
>  create mode 100644 handlers/readback_handler.c
> 
> diff --git a/handlers/Config.in b/handlers/Config.in
> index 41eac1c..5a03740 100644
> --- a/handlers/Config.in
> +++ b/handlers/Config.in
> @@ -106,6 +106,13 @@ config RDIFFHANDLER
>  	  Add support for applying librsync's rdiff patches,
>  	  see http://librsync.sourcefrog.net/
>  
> +config READBACKHANDLER
> +	bool "readback"
> +	select HASH_VERIFY

But this could not be possible if SSL_IMPL_OPENSSL or SSL_IMPL_MBEDTLS
are not set. What happen then ? I had just used "depends", not "select"

> +	default n
> +	help
> +	  Use sha256 hash to verify a target partition.
> +

The comment is very scarce - we coulf understand what the handler does
from the commit message, but not from the comment here. And rather the
commit message is hidden in git history, and it does not appear during a
"menuconfig". Please extend it to say:

- description as in commit message
- explain this is a post-install handler

>  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..2e4faa8
> --- /dev/null
> +++ b/handlers/readback_handler.c
> @@ -0,0 +1,161 @@
> +/*
> + * SPDX-FileCopyrightText: 2020 Bosch Sicherheitssysteme GmbH
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.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 verify_file_hash(const char* filename, size_t size, long offset, unsigned char *hash);
> +
> +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 file hash */
> +	char *ascii_hash = dict_get_value(&img->properties, "sha256");
> +	if (!ascii_hash) {

Check can be done here instead of later:
	if (!ascii_hash || ascii_to_hash(hash, ascii_hash) < 0 ||
!IsValidHash(hash))


> +		ERROR("Property sha256 not found");
> +		return -EINVAL;
> +	}
> +
> +	/* Get file size */
> +	char *value = dict_get_value(&img->properties, "size");
> +	if (!value) {
> +		ERROR("Property size not found");
> +		return -EINVAL;
> +	}
> +	unsigned long long size = ustrtoull(value, 10);
> +
> +	/* Get file offset */
> +	value = dict_get_value(&img->properties, "offset");
> +	if (!value) {
> +		ERROR("Property offset not found");
> +		return -EINVAL;
> +	}

I will suggest to have default values, too. If "offset" is not set into
sw-description, it should be set to 0 and you add a TRACE or WARN here.

In case of "size", if it is not present in sw-description, it should be
set to the size of the device i.e. partition to be read.

> +	unsigned long long offset = ustrtoull(value, 10);
> +
> +	/* Convert the ascii hash to binary */
> +	unsigned char hash[SHA256_HASH_LENGTH];
> +	ascii_to_hash(hash, ascii_hash);

all returns values must be checked for error. ascii_to_hash returns < 0
(-EINVAL) in case of error

> +
> +	return verify_file_hash(img->device, size, offset, hash);
> +}
> +
> +#define BUF_SIZE 4096
> +
> +static int verify_file_hash(const char* filename, size_t size, long offset, unsigned char *hash)
> +{
> +	int status = 0;
> +	FILE *fp = NULL;
> +	unsigned char *buf = NULL;
> +	struct swupdate_digest *dgst = NULL;
> +
> +	if (IsValidHash(hash) == 0) {
> +		ERROR("Invalid hash");
> +		return -EINVAL;
> +	}
> +
> +	if (!filename) {
> +		ERROR("Invalid file name");
> +		return -EINVAL;
> +	}
> +
> +	fp = fopen(filename, "rb");
> +	if (!fp) {
> +		ERROR("Failed to open %s: %s", filename, strerror(errno));
> +		status = -ENODEV;
> +		goto cleanup;
> +	}
> +
> +	if (fseek(fp, offset, SEEK_SET)) {
> +		ERROR("Failed to seek to position %ld: %s", offset, strerror(errno));
> +		status = -ENODEV;
> +		goto cleanup;
> +	}
> +
> +	dgst = swupdate_HASH_init(SHA_DEFAULT);
> +	if (!dgst) {
> +		status = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +	buf = malloc(BUF_SIZE);
> +	if (!buf) {
> +		status = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	size_t totalread = 0;
> +	while (totalread < size) {
> +		memset(buf, 0, BUF_SIZE);
> +

Do you really need to implement yourself this ? Can we use common code ?
What about to:

- instantiate a img structure
- set appropriate fields (size, hash, etc.)
- use as output file "/dev/null"
- call copyimage(). This already returns an error if hash does not match.

Best regards,
Stefano Babic

> +		size_t readsize = ((size - totalread) > BUF_SIZE) ?
> +				BUF_SIZE : (size - totalread);
> +
> +		if (fread(buf, 1, readsize, fp) != readsize) {
> +			ERROR("Failed to read %s: %s", filename, strerror(errno));
> +			status = -EIO;
> +			goto cleanup;
> +		}
> +		totalread += readsize;
> +
> +		if (swupdate_HASH_update(dgst, buf, readsize) < 0) {
> +			status = -EFAULT;
> +			goto cleanup;
> +		}
> +	}
> +
> +	unsigned int hash_len = 0;
> +	unsigned char hash_value[SHA256_HASH_LENGTH];
> +	if (swupdate_HASH_final(dgst, hash_value, &hash_len) < 0) {
> +		status = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +	if (hash_len != SHA256_HASH_LENGTH || swupdate_HASH_compare(hash_value, hash) < 0) {
> +		ERROR("Hash does not match");
> +		status = -EFAULT;
> +		goto cleanup;
> +	}
> +
> +	INFO("Verify file hash success: %s", filename);
> +
> +cleanup:
> +	if (fp)
> +		fclose(fp);
> +	if (buf)
> +		free(buf);
> +	if (dgst)
> +		swupdate_HASH_cleanup(dgst);
> +
> +	return status;
> +}
> +
> +__attribute__((constructor))
> +void readback_handler(void)
> +{
> +	register_handler("readback", readback, SCRIPT_HANDLER | NO_DATA_HANDLER, NULL);
> +}
>

Hi Stefano,

thank you for your feedback.

> > +config READBACKHANDLER
> > +	bool "readback"
> > +	select HASH_VERIFY
> 
> But this could not be possible if SSL_IMPL_OPENSSL or SSL_IMPL_MBEDTLS
> are not set. What happen then ? I had just used "depends", not "select"

Good point. We changed it to "depends HASH_VERIFY".

> > +	default n
> > +	help
> > +	  Use sha256 hash to verify a target partition.
> > +
> 
> The comment is very scarce - we coulf understand what the handler does
> from the commit message, but not from the comment here. And rather the
> commit message is hidden in git history, and it does not appear during a
> "menuconfig". Please extend it to say:
> 
> - description as in commit message
> - explain this is a post-install handler

The text is now more elaborate.

> > +static int readback_postinst(struct img_type *img) {
> > +	/* Get file hash */
> > +	char *ascii_hash = dict_get_value(&img->properties, "sha256");
> > +	if (!ascii_hash) {
> 
> Check can be done here instead of later:
> 	if (!ascii_hash || ascii_to_hash(hash, ascii_hash) < 0 ||
> !IsValidHash(hash))

Changed according to proposal.

> > +	/* Get file size */
> > +	char *value = dict_get_value(&img->properties, "size");
> > +	if (!value) {
> > +		ERROR("Property size not found");
> > +		return -EINVAL;
> > +	}
> > +	unsigned long long size = ustrtoull(value, 10);
> > +
> > +	/* Get file offset */
> > +	value = dict_get_value(&img->properties, "offset");
> > +	if (!value) {
> > +		ERROR("Property offset not found");
> > +		return -EINVAL;
> > +	}
> 
> I will suggest to have default values, too. If "offset" is not set into sw-
> description, it should be set to 0 and you add a TRACE or WARN here.
> 
> In case of "size", if it is not present in sw-description, it should be set to the
> size of the device i.e. partition to be read.

offset is now an optional property with a default value of 0. A TRACE will
be printed in case the default value is used.

size is still a mandatory parameter because we are not sure how to
properly retrieve the size of a partition.

stat() always returns st_size = 0 for block devices.

Using fseek(fp, 0, SEEK_END) and ftell(fp) works but looks awkward.

What's the recommended way to do it?

> > +	unsigned long long offset = ustrtoull(value, 10);
> > +
> > +	/* Convert the ascii hash to binary */
> > +	unsigned char hash[SHA256_HASH_LENGTH];
> > +	ascii_to_hash(hash, ascii_hash);
> 
> all returns values must be checked for error. ascii_to_hash returns < 0
> (-EINVAL) in case of error

We are now doing that.

> > +static int verify_file_hash(const char* filename, size_t size, long
> > +offset, unsigned char *hash) {
[..]
> Do you really need to implement yourself this ? Can we use common code
> ?
> What about to:
> 
> - instantiate a img structure
> - set appropriate fields (size, hash, etc.)
> - use as output file "/dev/null"
> - call copyimage(). This already returns an error if hash does not match.

copyimage() and copyfile() do not have the possibility to specify an
input offset. Would it be ok to seek the input fd upfront and pass it
then to copyfile()?

Tuesday is the last working day before Chinese New Year. We are trying
to close these two open points (size parameter and copyfile() reuse)
until then and send a new patch. If not I kindly ask you to be patient
with us until early February.

Mit freundlichen Grüßen / Best regards

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, Kevin,

On 17/01/20 13:28, 'Jonas Mark (BT-FIR/ENG1-Grb)' via swupdate wrote:
> Hi Stefano,
> 
> thank you for your feedback.
> 
>>> +config READBACKHANDLER
>>> +	bool "readback"
>>> +	select HASH_VERIFY
>>
>> But this could not be possible if SSL_IMPL_OPENSSL or SSL_IMPL_MBEDTLS
>> are not set. What happen then ? I had just used "depends", not "select"
> 
> Good point. We changed it to "depends HASH_VERIFY".
> 

ok, thanks.

>>> +	default n
>>> +	help
>>> +	  Use sha256 hash to verify a target partition.
>>> +
>>
>> The comment is very scarce - we coulf understand what the handler does
>> from the commit message, but not from the comment here. And rather the
>> commit message is hidden in git history, and it does not appear during a
>> "menuconfig". Please extend it to say:
>>
>> - description as in commit message
>> - explain this is a post-install handler
> 
> The text is now more elaborate.
> 
>>> +static int readback_postinst(struct img_type *img) {
>>> +	/* Get file hash */
>>> +	char *ascii_hash = dict_get_value(&img->properties, "sha256");
>>> +	if (!ascii_hash) {
>>
>> Check can be done here instead of later:
>> 	if (!ascii_hash || ascii_to_hash(hash, ascii_hash) < 0 ||
>> !IsValidHash(hash))
> 
> Changed according to proposal.
> 
>>> +	/* Get file size */
>>> +	char *value = dict_get_value(&img->properties, "size");
>>> +	if (!value) {
>>> +		ERROR("Property size not found");
>>> +		return -EINVAL;
>>> +	}
>>> +	unsigned long long size = ustrtoull(value, 10);
>>> +
>>> +	/* Get file offset */
>>> +	value = dict_get_value(&img->properties, "offset");
>>> +	if (!value) {
>>> +		ERROR("Property offset not found");
>>> +		return -EINVAL;
>>> +	}
>>
>> I will suggest to have default values, too. If "offset" is not set into sw-
>> description, it should be set to 0 and you add a TRACE or WARN here.
>>
>> In case of "size", if it is not present in sw-description, it should be set to the
>> size of the device i.e. partition to be read.
> 
> offset is now an optional property with a default value of 0. A TRACE will
> be printed in case the default value is used.

Fine.

> 
> size is still a mandatory parameter because we are not sure how to
> properly retrieve the size of a partition.
> 
> stat() always returns st_size = 0 for block devices.
> 
> Using fseek(fp, 0, SEEK_END) and ftell(fp) works but looks awkward.
> 
> What's the recommended way to do it?
> 
>>> +	unsigned long long offset = ustrtoull(value, 10);
>>> +
>>> +	/* Convert the ascii hash to binary */
>>> +	unsigned char hash[SHA256_HASH_LENGTH];
>>> +	ascii_to_hash(hash, ascii_hash);
>>
>> all returns values must be checked for error. ascii_to_hash returns < 0
>> (-EINVAL) in case of error
> 
> We are now doing that.
> 
>>> +static int verify_file_hash(const char* filename, size_t size, long
>>> +offset, unsigned char *hash) {
> [..]
>> Do you really need to implement yourself this ? Can we use common code
>> ?
>> What about to:
>>
>> - instantiate a img structure
>> - set appropriate fields (size, hash, etc.)
>> - use as output file "/dev/null"
>> - call copyimage(). This already returns an error if hash does not match.
> 
> copyimage() and copyfile() do not have the possibility to specify an
> input offset. Would it be ok to seek the input fd upfront and pass it
> then to copyfile()?

Yes, you do not need an offset. copyfile() and copyimage() accepts a
stream (usually a network stream) as file descriptor. If you pass the fd
pointing to the right offset, that's all. So you open "filename" as in
your patch (but you have to use file descriptor instead of file pointer)
and seek it, then pass it to copyimage as img->fdin.

> 
> Tuesday is the last working day before Chinese New Year.

Well, let me wish to Kevin and all chinese developers a happy new year !

> We are trying
> to close these two open points (size parameter and copyfile() reuse)
> until then and send a new patch. If not I kindly ask you to be patient
> with us until early February.

No problem on my site.

Best regards,
Stefano

On 17/01/20 13:42, Stefano Babic wrote:
> Hi Mark, Kevin,
> 
> On 17/01/20 13:28, 'Jonas Mark (BT-FIR/ENG1-Grb)' via swupdate wrote:
>> Hi Stefano,
>>
>> thank you for your feedback.
>>
>>>> +config READBACKHANDLER
>>>> +	bool "readback"
>>>> +	select HASH_VERIFY
>>>
>>> But this could not be possible if SSL_IMPL_OPENSSL or SSL_IMPL_MBEDTLS
>>> are not set. What happen then ? I had just used "depends", not "select"
>>
>> Good point. We changed it to "depends HASH_VERIFY".
>>
> 
> ok, thanks.
> 
>>>> +	default n
>>>> +	help
>>>> +	  Use sha256 hash to verify a target partition.
>>>> +
>>>
>>> The comment is very scarce - we coulf understand what the handler does
>>> from the commit message, but not from the comment here. And rather the
>>> commit message is hidden in git history, and it does not appear during a
>>> "menuconfig". Please extend it to say:
>>>
>>> - description as in commit message
>>> - explain this is a post-install handler
>>
>> The text is now more elaborate.
>>
>>>> +static int readback_postinst(struct img_type *img) {
>>>> +	/* Get file hash */
>>>> +	char *ascii_hash = dict_get_value(&img->properties, "sha256");
>>>> +	if (!ascii_hash) {
>>>
>>> Check can be done here instead of later:
>>> 	if (!ascii_hash || ascii_to_hash(hash, ascii_hash) < 0 ||
>>> !IsValidHash(hash))
>>
>> Changed according to proposal.
>>
>>>> +	/* Get file size */
>>>> +	char *value = dict_get_value(&img->properties, "size");
>>>> +	if (!value) {
>>>> +		ERROR("Property size not found");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +	unsigned long long size = ustrtoull(value, 10);
>>>> +
>>>> +	/* Get file offset */
>>>> +	value = dict_get_value(&img->properties, "offset");
>>>> +	if (!value) {
>>>> +		ERROR("Property offset not found");
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>> I will suggest to have default values, too. If "offset" is not set into sw-
>>> description, it should be set to 0 and you add a TRACE or WARN here.
>>>
>>> In case of "size", if it is not present in sw-description, it should be set to the
>>> size of the device i.e. partition to be read.
>>
>> offset is now an optional property with a default value of 0. A TRACE will
>> be printed in case the default value is used.
> 
> Fine.
> 
>>
>> size is still a mandatory parameter because we are not sure how to
>> properly retrieve the size of a partition.
>>
>> stat() always returns st_size = 0 for block devices.
>>
>> Using fseek(fp, 0, SEEK_END) and ftell(fp) works but looks awkward.
>>
>> What's the recommended way to do it?
>>

I already done this in the raw handler, you can copy from it:

 163         if (ioctl(fdin, BLKGETSIZE64, &size) < 0) {
 164                 ERROR("Cannot get size of %s", entry->value);
 165         }
 166

What I uncertain is what happens in case of UBI block device - the code
above is in the raw handler, generic block device. Theoretically it
should work for UBI block devices, too, but I have not tested it.

Regards,
Stefano