[PATCH 2/2] - add optional checksum verification before installation

[Stefano Babic]

From: Bernhard Breinbauer <bernhard....@wallner-automation.com>

---
archival/Config.src | 10 ++++++++++
core/cpio_utils.c | 39 +++++++++++++++++++++++++++++++++++++++
core/swupdate.c | 6 +++++-
include/util.h | 1 +
4 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/archival/Config.src b/archival/Config.src
index 3978ced..8d08da6 100644
--- a/archival/Config.src
+++ b/archival/Config.src
@@ -21,6 +21,16 @@ config CPIO
Unless you have a specific application which requires cpio, you
should probably say N here.

+config CPIO_VERIFY_CHECKSUM_BEFORE_UPDATE
+ bool "cpio: verify checksum before update"
+ default y
+ help
+ If activated all of the cpio archive's checksums are verified before
+ starting the update. Otherwise the checksums are verified while/after
+ the update installation.
+ As the pre-installation checksum verification takes some time, this
+ behavior can be deactivated
+
config GUNZIP
bool "gunzip"
default y
diff --git a/core/cpio_utils.c b/core/cpio_utils.c
index 877eb1e..26eaf5b 100644
--- a/core/cpio_utils.c
+++ b/core/cpio_utils.c
@@ -206,6 +206,30 @@ int copyfile(int fdin, int fdout, int nbytes, unsigned long *offs,
return 0;
}

+int calc_checksum(int fdin, int nbytes, unsigned long offset,
+ uint32_t *checksum)
+{
+ unsigned long size;
+ int ret;
+
+ if (checksum)
+ *checksum = 0;
+
+ while (nbytes > 0) {
+ size = (nbytes < BUFF_SIZE ? nbytes : BUFF_SIZE);
+
+ if ((ret = fill_buffer(fdin, in, size, &offset, checksum) < 0)) {
+ return ret;
+ }
+
+ nbytes -= size;
+ }
+
+ fill_buffer(fdin, in, NPAD_BYTES(offset), &offset, checksum);
+
+ return 0;
+}
+
int extract_cpio_header(int fd, struct filehdr *fhdr, unsigned long *offset)
{
unsigned char buf[256];
@@ -348,6 +372,9 @@ off_t cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start)
struct filehdr fdh;
unsigned long offset = start;
int file_listed;
+#ifdef CONFIG_CPIO_VERIFY_CHECKSUM_BEFORE_UPDATE
+ uint32_t checksum;
+#endif

while (1) {
file_listed = 0;
@@ -371,6 +398,18 @@ off_t cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start)
(unsigned int)fdh.size,
file_listed ? "REQUIRED" : "not required");

+#ifdef CONFIG_CPIO_VERIFY_CHECKSUM_BEFORE_UPDATE
+ if (calc_checksum(fd, fdh.size, offset, &checksum) != 0) {
+ TRACE("Checksum calculation failed for %s\n",
+ fdh.filename);
+ return -1;
+ }
+ if ((uint32_t)(fdh.chksum) != checksum) {
+ TRACE("Checksum verification failed for %s: %x != %x\n",
+ fdh.filename, (uint32_t)fdh.chksum, checksum);
+ return -1;
+ }
+#endif
/* Skip to the end of the file */
offset += fdh.size;

diff --git a/core/swupdate.c b/core/swupdate.c
index 897d896..4eb1811 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -219,7 +219,11 @@ static int install_from_file(char *fname)
exit(1);
}

- cpio_scan(fdsw, &swcfg, pos);
+ pos = cpio_scan(fdsw, &swcfg, pos);
+ if (pos < 0) {
+ close(fdsw);
+ exit(1);
+ }

/*
* Check if all files described in sw-description
diff --git a/include/util.h b/include/util.h
index 65ed6f2..511dbf8 100644
--- a/include/util.h
+++ b/include/util.h
@@ -119,6 +119,7 @@ int copyfile(int fdin, int fdout, int nbytes, unsigned long *offs,
off_t extract_sw_description(int fd);
off_t extract_next_file(int fd, int fdout, off_t start, int compressed);
int openfileoutput(const char *filename);
+int calc_checksum(int fdin, int nbytes, unsigned long offset, uint32_t *checksum);

int register_notifier(notifier client);
void notify(RECOVERY_STATUS status, int error, const char *msg);

[Stefano Babic]

From: Bernhard Breinbauer <bernhard....@wallner-automation.com>

---

handlers/raw_handler.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/handlers/raw_handler.c b/handlers/raw_handler.c
index c264337..d1b849d 100644
--- a/handlers/raw_handler.c
+++ b/handlers/raw_handler.c
@@ -57,6 +57,11 @@ static int install_raw_image(struct img_type *img,
}

ret = copyfile(img->fdin, fdout, img->size, &offset, 0, img->compressed, &checksum);
+ if ((uint32_t)(img->checksum) != checksum)
+ {
+ TRACE("Checkum verification failed: %x != %x\n", img->checksum, checksum);
+ ret = -1;
+ }
close(fdout);
return ret;
}

[Stefano Babic]

Hi Bernhard,

thanks for patch. However, please send patches to this ML and not to
github. I am trying to manage this project as usual in FLOSS world, and
posting to ML allow someone else to take a look.

Please add your Signed-off-by to the patches you send - your
contribution should be tracked :-).

This does not work when the image is streamed from network. In fact,
img->checksum is not set. However, the issue you report does not happen
when the update is coming from network, because the single images are
extracted and store temporarily on /tmp, and for each of them the
checksum is computed before copying.

But again, adding this to the raw handler breaks the installation from
network.

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: sba...@denx.de
=====================================================================

[Stefano Babic]

Hi Bernhard,

On 01/04/2015 15:18, Stefano Babic wrote:
> From: Bernhard Breinbauer <bernhard....@wallner-automation.com>
>

Let's explain your issue - you have posted :

"Looks like I'm the only one with bad luck and an usb flash drive which
reliably corrupts data...
This patch adds checksum verification for the raw_handler and an
optional checksum verification for the whole archive before the
installation of the update starts."

This means that the installation fails with an error, but part of the
software was already installed on the system, destroying the old
software. Right ?

You are not alone about bad drive, but the issue with corrupt data on
USB flash is not solved at all computing the checksum before installing.

In fact, if you compute the checksum on the whole image before
installing, it does not guarantee that the extracted image will be
correct later, when the corresponding handler is performing the
installation. You add also the test for the raw handler, but as I said
on the previous patch, this breaks the installation from network.

On the other side, if there will be a check, this should be done
independently from the handler that is called. Maybe you are using only
the raw handler, but installing with a different handler from you bad
USB stick will fail as well, I suppose.

> ---
> archival/Config.src | 10 ++++++++++
> core/cpio_utils.c | 39 +++++++++++++++++++++++++++++++++++++++
> core/swupdate.c | 6 +++++-
> include/util.h | 1 +
> 4 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/archival/Config.src b/archival/Config.src
> index 3978ced..8d08da6 100644
> --- a/archival/Config.src
> +++ b/archival/Config.src
> @@ -21,6 +21,16 @@ config CPIO
> Unless you have a specific application which requires cpio, you
> should probably say N here.
>
> +config CPIO_VERIFY_CHECKSUM_BEFORE_UPDATE
> + bool "cpio: verify checksum before update"
> + default y
> + help
> + If activated all of the cpio archive's checksums are verified before
> + starting the update. Otherwise the checksums are verified while/after
> + the update installation.
> + As the pre-installation checksum verification takes some time, this
> + behavior can be deactivated
> +

This is a change in behavior and it is not strictly related to a target
configuration. Maybe we can handle it better if we add a comman line
parameter for it instead of a CONFIG_ option.

It is a copy of copyfile() function without the real copy. Maybe can we
avoid to duplicate code ?

> +
> int extract_cpio_header(int fd, struct filehdr *fhdr, unsigned long *offset)
> {
> unsigned char buf[256];
> @@ -348,6 +372,9 @@ off_t cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start)
> struct filehdr fdh;
> unsigned long offset = start;
> int file_listed;
> +#ifdef CONFIG_CPIO_VERIFY_CHECKSUM_BEFORE_UPDATE
> + uint32_t checksum;
> +#endif
>
> while (1) {
> file_listed = 0;
> @@ -371,6 +398,18 @@ off_t cpio_scan(int fd, struct swupdate_cfg *cfg, off_t start)
> (unsigned int)fdh.size,
> file_listed ? "REQUIRED" : "not required");
>

cpio_scan() is called only if installed from a file. That's ok.

[Bernhard Breinbauer]

Hi Stefano,

I will post patches to the ML in the future, thanks.
See my coments below.

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic [mailto:sba...@denx.de]
> Gesendet: Mittwoch, 1. April 2015 15:36
> An: Stefano Babic; swup...@googlegroups.com
> Cc: Bernhard Breinbauer
> Betreff: Re: [swupdate] [PATCH 2/2] - add optional checksum verification before
> installation

>
> Hi Bernhard,
>
> On 01/04/2015 15:18, Stefano Babic wrote:
> > From: Bernhard Breinbauer <bernhard....@wallner-automation.com>
> >
>
> Let's explain your issue - you have posted :
>
> "Looks like I'm the only one with bad luck and an usb flash drive which
> reliably corrupts data...
> This patch adds checksum verification for the raw_handler and an
> optional checksum verification for the whole archive before the
> installation of the update starts."
>
> This means that the installation fails with an error, but part of the
> software was already installed on the system, destroying the old
> software. Right ?

No, the installation does not report an error. The checksum is never validated
(except for the sw-description file).

> You are not alone about bad drive, but the issue with corrupt data on
> USB flash is not solved at all computing the checksum before installing.

IMO an integrity test of an update package is actually mandatory and should
be performed before installing the image (if technically possible).

> In fact, if you compute the checksum on the whole image before
> installing, it does not guarantee that the extracted image will be
> correct later, when the corresponding handler is performing the
> installation. You add also the test for the raw handler, but as I said
> on the previous patch, this breaks the installation from network.

I'm sorry. I'm not familiar with the installation from network and also not
very familiar with the internals of swupdate.
How is the image changed in the process, so that checksum verification
will fail?
Why is the checksum field not set when it is streamed?

> On the other side, if there will be a check, this should be done
> independently from the handler that is called. Maybe you are using only
> the raw handler, but installing with a different handler from you bad
> USB stick will fail as well, I suppose.

Yes, I only changed the raw handler, because that is what I'm using and
what I can test with my setup.

[Stefano Babic]

Hi Bernhard,

Ouch...something went terribly wrong. This must be fixed.

> I'm sorry. I'm not familiar with the installation from network and also not
> very familiar with the internals of swupdate.
> How is the image changed in the process, so that checksum verification
> will fail?
> Why is the checksum field not set when it is streamed?

Because the big image is streamed from network, swupdate must extract
the relevant sub-images and then copies them into temporary area before
passing them to the handler. By copying the checksum is proofed.

You see this in stream_interface.c:

190 if (copyfile(fd, fdout, fdh.size, &offset,
skip, 0, &checksum) < 0) {
191 return -1;
192 }
193 close(fdout);
194 if (checksum != (unsigned long)fdh.chksum) {
195 TRACE("Checksum WRONG ! Computed
0x%ux, it should be 0x%ux",
196 (unsigned int)checksum,
(unsigned int)fdh.chksum);
197 return -1;
198 }

>
>> On the other side, if there will be a check, this should be done
>> independently from the handler that is called. Maybe you are using only
>> the raw handler, but installing with a different handler from you bad
>> USB stick will fail as well, I suppose.
> Yes, I only changed the raw handler, because that is what I'm using and
> what I can test with my setup.

Yes, I understand it.

However, I supposed that checksum was already verified also in case of
installation from file. That means this is a bug and the checksum must
*always* be verified.

[Bernhard Breinbauer]

Hi Stefano,

I think we are getting to understand each other better ;)
And it looks like we have orthogonal test setups, so let's find a common
ground for checksum verification.

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic [mailto:sba...@denx.de]

> Gesendet: Donnerstag, 2. April 2015 10:18
> An: Bernhard Breinbauer; Stefano Babic; swup...@googlegroups.com
> Betreff: Re: AW: [swupdate] [PATCH 2/2] - add optional checksum verification

OK. I understand and that also explains why the change in raw_handler breaks
network installation.
Where would be the correct place to add the checksum validation? I'm open to change
the patch accordingly.

Thinking about it, I like the idea adding a command line parameter.

[Stefano Babic]

Hi Bernhard,

On 02/04/2015 11:14, Bernhard Breinbauer wrote:
> Hi Stefano,
>

> I think we are getting to understand each other better ;)

Sure, this is how open source works !

> And it looks like we have orthogonal test setups, so let's find a common
> ground for checksum verification.
>
>> -----Ursprüngliche Nachricht-----
>> Von: Stefano Babic [mailto:sba...@denx.de]
>> Gesendet: Donnerstag, 2. April 2015 10:18
>> An: Bernhard Breinbauer; Stefano Babic; swup...@googlegroups.com
>> Betreff: Re: AW: [swupdate] [PATCH 2/2] - add optional checksum verification
>> before installation

>> You see this in stream_interface.c:
>>
>> 190 if (copyfile(fd, fdout, fdh.size, &offset,
>> skip, 0, &checksum) < 0) {
>> 191 return -1;
>> 192 }
>> 193 close(fdout);
>> 194 if (checksum != (unsigned long)fdh.chksum) {
>> 195 TRACE("Checksum WRONG ! Computed
>> 0x%ux, it should be 0x%ux",
>> 196 (unsigned int)checksum,
>> (unsigned int)fdh.chksum);
>> 197 return -1;
>> 198 }
>
> OK. I understand and that also explains why the change in raw_handler breaks
> network installation.
> Where would be the correct place to add the checksum validation? I'm open to change
> the patch accordingly.

IMHO checkum verification is a *must* and it is a pity (really, a *BUG*)
that it does not work when installing from file as everyone expects. I
would add the verification *always*, even if it takes some more time. By
swupdate it is more important that it works safely as it works fast.
During an upgrade, it is quite normal that this takes some time. And the
system must reboot after upgrading, and this takes time, too, surely
much more as a checksum verification.

I agree adding the check in cpio_scan(). Anyway, what about using the
copyfile function ? You can use it as it is if you take as file output
(fdout) "/dev/null". The result is like reading the whole image without
writing and returning the checksum.

>> However, I supposed that checksum was already verified also in case of
>> installation from file. That means this is a bug and the checksum must
>> *always* be verified.
> Thinking about it, I like the idea adding a command line parameter.

Well, I propose this when I was thinking that checksum was already
proofed. I am aware you have found a bug: as checksum is verified when
installing from network, it must also always be verified when installing
from file.

[Bernhard Breinbauer]

Hi Stefano,

> -----Ursprüngliche Nachricht-----
> Von: Stefano Babic [mailto:sba...@denx.de]

> Gesendet: Donnerstag, 2. April 2015 16:49

> An: Bernhard Breinbauer; Stefano Babic; swup...@googlegroups.com

> Betreff: Re: AW: AW: [swupdate] [PATCH 2/2] - add optional checksum verification

OK. Let's do it in cpio_scan() and I will use copyfile() instead.
One more question: The install_from_file() function will not be used when streaming
is implemented, correct()? So I will add a parameter verify to cpio_scan(), which enables
the checksum verification and can be used from install_from_file().

[Stefano Babic]

Hi Bernhard,

On 03/04/2015 08:34, Bernhard Breinbauer wrote:

>> I agree adding the check in cpio_scan(). Anyway, what about using the copyfile
>> function ? You can use it as it is if you take as file output
>> (fdout) "/dev/null". The result is like reading the whole image without writing and
>> returning the checksum.
> OK. Let's do it in cpio_scan() and I will use copyfile() instead.
> One more question: The install_from_file() function will not be used when streaming
> is implemented, correct()?

Correct.