| Message ID | cb32aba5-024f-2a29-cb76-08c2188e05ec@streamunlimited.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [meta-swupdate] cpio_utils: verify also image hash while scanning cpio | expand |
Hi Martin, On 18/03/2018 14:24, Martin Geier wrote: > From 33e94add7fdb74d96cca812da2552501111cfcf9 Mon Sep 17 00:00:00 2001 > From: Martin Geier <martin.geier@streamunlimited.com> > Date: Sun, 18 Mar 2018 13:39:25 +0100 > Subject: [meta-swupdate][PATCH] cpio_utils: verify also image hash while ^----- patch is for SWUpdate, not for Yocto's layer. > scanning cpio > > When update from local file is performed, files hashes are verified > only during cpio_utils::copyfile called from fs handler and not before. > If fs handler (ubi) is not extracting file to ram before writing, > unsigned file can be write to fs. Anyway, this is the common case. If "install-directly", SWUpdate works in stream mode and no previous check is done - it is not possible in case the image is coming from network. > > Signed-off-by: Martin Geier <martin.geier@streamunlimited.com> > --- > core/cpio_utils.c | 15 +++++++-------- > include/swupdate.h | 18 ++++++++++-------- > 2 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/core/cpio_utils.c b/core/cpio_utils.c > index 15c1374..1543772 100644 > --- a/core/cpio_utils.c > +++ b/core/cpio_utils.c > @@ -647,12 +647,10 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, > off_t start) > return 0; > } > > - SEARCH_FILE(struct img_type, cfg->images, > - file_listed, start); > - SEARCH_FILE(struct img_type, cfg->scripts, > - file_listed, start); > - SEARCH_FILE(struct img_type, cfg->bootscripts, > - file_listed, start); > + struct img_type *img = NULL; > + SEARCH_FILE(img, cfg->images, file_listed, start); > + SEARCH_FILE(img, cfg->scripts, file_listed, start); > + SEARCH_FILE(img, cfg->bootscripts, file_listed, start); I do not see any change in the code. What does this mean ? If this is a formal cjange in formatting, it must be addressed by a separate patch. Rule is, as usual, one patch for one issue. > > TRACE("Found file:\n\tfilename %s\n\tsize %lu\n\t%s\n", > fdh.filename, > @@ -660,10 +658,11 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, > off_t start) > file_listed ? "REQUIRED" : "not required"); > > /* > - * use copyfile for checksum verification, as we skip file > + * use copyfile for checksum and hash verification, as we skip > file > * we do not have to provide fdout > */ > - if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, > NULL, 0, NULL) != 0) { > + if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, > img ? img->sha256 : NULL, > + 0, NULL) != 0) { IMHO the issue on your patch is addressed by these changes above - other changes are unrelated and shouldn't be part of the patch. > ERROR("invalid archive\n"); > return -1; > } > diff --git a/include/swupdate.h b/include/swupdate.h > index 7de096a..e3e65e6 100644 > --- a/include/swupdate.h > +++ b/include/swupdate.h > @@ -129,18 +129,20 @@ struct swupdate_cfg { > const char *embscript; > }; > > -#define SEARCH_FILE(type, list, found, offs) do { \ > +#define SEARCH_FILE(img, list, found, offs) do { \ > if (!found) { \ > - type *p; \ > - for (p = list.lh_first; p != NULL; \ > - p = p->next.le_next) { \ > - if (strcmp(p->fname, fdh.filename) == 0) { \ > + for (img = list.lh_first; img != NULL; \ > + img = img->next.le_next) { \ > + if (strcmp(img->fname, fdh.filename) == 0) { \ > found = 1; \ > - p->offset = offs; \ > - p->provided = 1; \ > - p->size = fdh.size; \ > + img->offset = offs; \ > + img->provided = 1; \ > + img->size = fdh.size; \ > + break; \ > } \ > } \ > + if (!found) \ > + img = NULL; \ > } \ What is supposed to do in SEARCH_FILE ? Best regards, Stefano Babic -- ===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de =====================================================================
Hi Stefano, >> Subject: [meta-swupdate][PATCH] cpio_utils: verify also image hash while > ^----- patch is for SWUpdate, not for Yocto's layer. Sorry, my mistake > >> >> - SEARCH_FILE(struct img_type, cfg->images, >> - file_listed, start); >> - SEARCH_FILE(struct img_type, cfg->scripts, >> - file_listed, start); >> - SEARCH_FILE(struct img_type, cfg->bootscripts, >> - file_listed, start); >> + struct img_type *img = NULL; >> + SEARCH_FILE(img, cfg->images, file_listed, start); >> + SEARCH_FILE(img, cfg->scripts, file_listed, start); >> + SEARCH_FILE(img, cfg->bootscripts, file_listed, start); This changes adds "struct img_type *img = NULL;" >> - if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, >> NULL, 0, NULL) != 0) { >> + if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, >> img ? img->sha256 : NULL, >> + 0, NULL) != 0) { After those changes, you can use "img ? img->sha256 : NULL" and verify the file hash >> ERROR("invalid archive\n"); >> return -1; >> } >> diff --git a/include/swupdate.h b/include/swupdate.h >> index 7de096a..e3e65e6 100644 >> --- a/include/swupdate.h >> +++ b/include/swupdate.h >> @@ -129,18 +129,20 @@ struct swupdate_cfg { >> const char *embscript; >> }; >> >> -#define SEARCH_FILE(type, list, found, offs) do { \ >> +#define SEARCH_FILE(img, list, found, offs) do { \ >> if (!found) { \ >> - type *p; \ >> - for (p = list.lh_first; p != NULL; \ >> - p = p->next.le_next) { \ >> - if (strcmp(p->fname, fdh.filename) == 0) { \ >> + for (img = list.lh_first; img != NULL; \ >> + img = img->next.le_next) { \ >> + if (strcmp(img->fname, fdh.filename) == 0) { \ >> found = 1; \ >> - p->offset = offs; \ >> - p->provided = 1; \ >> - p->size = fdh.size; \ >> + img->offset = offs; \ >> + img->provided = 1; \ >> + img->size = fdh.size; \ >> + break; \ >> } \ >> } \ >> + if (!found) \ >> + img = NULL; \ >> } \ > What is supposed to do in SEARCH_FILE ? There is lots of ways how to retrieve sha256 from description file, this looked like a best way.
Hi Martin, On 18/03/2018 17:05, Martin Geier wrote: > Hi Stefano, > > >>> Subject: [meta-swupdate][PATCH] cpio_utils: verify also image hash while >> ^----- patch is for SWUpdate, not for Yocto's layer. > Sorry, my mistake >> >>> - SEARCH_FILE(struct img_type, cfg->images, >>> - file_listed, start); >>> - SEARCH_FILE(struct img_type, cfg->scripts, >>> - file_listed, start); >>> - SEARCH_FILE(struct img_type, cfg->bootscripts, >>> - file_listed, start); >>> + struct img_type *img = NULL; >>> + SEARCH_FILE(img, cfg->images, file_listed, start); >>> + SEARCH_FILE(img, cfg->scripts, file_listed, start); >>> + SEARCH_FILE(img, cfg->bootscripts, file_listed, start); > This changes adds "struct img_type *img = NULL;" >>> - if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, >>> NULL, 0, NULL) != 0) { >>> + if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, >>> img ? img->sha256 : NULL, >>> + 0, NULL) != 0) { > After those changes, you can use "img ? img->sha256 : NULL" Ok, I see. > and verify the file hash >>> ERROR("invalid archive\n"); >>> return -1; >>> } >>> diff --git a/include/swupdate.h b/include/swupdate.h >>> index 7de096a..e3e65e6 100644 >>> --- a/include/swupdate.h >>> +++ b/include/swupdate.h >>> @@ -129,18 +129,20 @@ struct swupdate_cfg { >>> const char *embscript; >>> }; >>> -#define SEARCH_FILE(type, list, found, offs) do { \ >>> +#define SEARCH_FILE(img, list, found, offs) do { \ >>> if (!found) { \ >>> - type *p; \ >>> - for (p = list.lh_first; p != NULL; \ >>> - p = p->next.le_next) { \ >>> - if (strcmp(p->fname, fdh.filename) == 0) { \ >>> + for (img = list.lh_first; img != NULL; \ >>> + img = img->next.le_next) { \ >>> + if (strcmp(img->fname, fdh.filename) == 0) { \ >>> found = 1; \ >>> - p->offset = offs; \ >>> - p->provided = 1; \ >>> - p->size = fdh.size; \ >>> + img->offset = offs; \ >>> + img->provided = 1; \ >>> + img->size = fdh.size; \ >>> + break; \ >>> } \ >>> } \ >>> + if (!found) \ >>> + img = NULL; \ >>> } \ >> What is supposed to do in SEARCH_FILE ? > There is lots of ways how to retrieve sha256 from description file, this > looked like a best way. ok, fine. I will test it, but patch looks fine.+ Best regards, Stefano Babic -- ===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de =====================================================================
Hi Martin, your patch does not apply on current master. Can you check and repost ? Thanks ! Best regards, Stefano Babic On 18/03/2018 23:48, Stefano Babic wrote: > Hi Martin, > > On 18/03/2018 17:05, Martin Geier wrote: >> Hi Stefano, >> >> >>>> Subject: [meta-swupdate][PATCH] cpio_utils: verify also image hash while >>> ^----- patch is for SWUpdate, not for Yocto's layer. >> Sorry, my mistake >>> >>>> - SEARCH_FILE(struct img_type, cfg->images, >>>> - file_listed, start); >>>> - SEARCH_FILE(struct img_type, cfg->scripts, >>>> - file_listed, start); >>>> - SEARCH_FILE(struct img_type, cfg->bootscripts, >>>> - file_listed, start); >>>> + struct img_type *img = NULL; >>>> + SEARCH_FILE(img, cfg->images, file_listed, start); >>>> + SEARCH_FILE(img, cfg->scripts, file_listed, start); >>>> + SEARCH_FILE(img, cfg->bootscripts, file_listed, start); >> This changes adds "struct img_type *img = NULL;" >>>> - if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, >>>> NULL, 0, NULL) != 0) { >>>> + if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, >>>> img ? img->sha256 : NULL, >>>> + 0, NULL) != 0) { >> After those changes, you can use "img ? img->sha256 : NULL" > > Ok, I see. > >> and verify the file hash >>>> ERROR("invalid archive\n"); >>>> return -1; >>>> } >>>> diff --git a/include/swupdate.h b/include/swupdate.h >>>> index 7de096a..e3e65e6 100644 >>>> --- a/include/swupdate.h >>>> +++ b/include/swupdate.h >>>> @@ -129,18 +129,20 @@ struct swupdate_cfg { >>>> const char *embscript; >>>> }; >>>> -#define SEARCH_FILE(type, list, found, offs) do { \ >>>> +#define SEARCH_FILE(img, list, found, offs) do { \ >>>> if (!found) { \ >>>> - type *p; \ >>>> - for (p = list.lh_first; p != NULL; \ >>>> - p = p->next.le_next) { \ >>>> - if (strcmp(p->fname, fdh.filename) == 0) { \ >>>> + for (img = list.lh_first; img != NULL; \ >>>> + img = img->next.le_next) { \ >>>> + if (strcmp(img->fname, fdh.filename) == 0) { \ >>>> found = 1; \ >>>> - p->offset = offs; \ >>>> - p->provided = 1; \ >>>> - p->size = fdh.size; \ >>>> + img->offset = offs; \ >>>> + img->provided = 1; \ >>>> + img->size = fdh.size; \ >>>> + break; \ >>>> } \ >>>> } \ >>>> + if (!found) \ >>>> + img = NULL; \ >>>> } \ >>> What is supposed to do in SEARCH_FILE ? >> There is lots of ways how to retrieve sha256 from description file, this >> looked like a best way. > > ok, fine. I will test it, but patch looks fine.+ > > Best regards, > Stefano Babic > -- ===================================================================== DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de =====================================================================
diff --git a/core/cpio_utils.c b/core/cpio_utils.c index 15c1374..1543772 100644 --- a/core/cpio_utils.c +++ b/core/cpio_utils.c @@ -647,12 +647,10 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start) return 0; } - SEARCH_FILE(struct img_type, cfg->images, - file_listed, start); - SEARCH_FILE(struct img_type, cfg->scripts, - file_listed, start); - SEARCH_FILE(struct img_type, cfg->bootscripts, - file_listed, start); + struct img_type *img = NULL; + SEARCH_FILE(img, cfg->images, file_listed, start); + SEARCH_FILE(img, cfg->scripts, file_listed, start); + SEARCH_FILE(img, cfg->bootscripts, file_listed, start); TRACE("Found file:\n\tfilename %s\n\tsize %lu\n\t%s\n", fdh.filename, @@ -660,10 +658,11 @@ int cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start) file_listed ? "REQUIRED" : "not required"); /* - * use copyfile for checksum verification, as we skip file + * use copyfile for checksum and hash verification, as we skip file * we do not have to provide fdout */ - if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, NULL, 0, NULL) != 0) { + if (copyfile(fd, NULL, fdh.size, &offset, 0, 1, 0, &checksum, img ? img->sha256 : NULL, + 0, NULL) != 0) { ERROR("invalid archive\n"); return -1; } diff --git a/include/swupdate.h b/include/swupdate.h index 7de096a..e3e65e6 100644 --- a/include/swupdate.h +++ b/include/swupdate.h @@ -129,18 +129,20 @@ struct swupdate_cfg { const char *embscript; }; -#define SEARCH_FILE(type, list, found, offs) do { \ +#define SEARCH_FILE(img, list, found, offs) do { \ if (!found) { \ - type *p; \ - for (p = list.lh_first; p != NULL; \ - p = p->next.le_next) { \ - if (strcmp(p->fname, fdh.filename) == 0) { \ + for (img = list.lh_first; img != NULL; \ + img = img->next.le_next) { \ + if (strcmp(img->fname, fdh.filename) == 0) { \ found = 1; \ - p->offset = offs; \ - p->provided = 1; \ - p->size = fdh.size; \ + img->offset = offs; \ + img->provided = 1; \ + img->size = fdh.size; \ + break; \ } \ } \ + if (!found) \ + img = NULL; \ } \ } while(0)