Message ID | 20200120124312.15129-2-mark.jonas@de.bosch.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Add readback handler for partition verify | expand |
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
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. 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)); + } + + /* 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 */ + 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); +}