[flashrom] [PATCH] Rigorously check integrity of I/O stream data.

Urja Rannikko urjaman at gmail.com
Sun Nov 29 16:40:06 CET 2015


Hi,

On Sun, Nov 22, 2015 at 7:43 PM, Stefan Tauner
<stefan.tauner at alumni.tuwien.ac.at> wrote:
> Even if fwrite() succeeds the data is not necessarily out of the clib's buffers
> and writing it eventually could fail. Even if the data is flushed out (explicitly by
> fflush() or implicitly by fclose()) the kernel might still hold a buffer.
>
> Previously we have ignored this to a large extent - even in important cases
> like writing the flash contents to a file. The results can be truncated
> images that would brick the respective machine if written back as is (though
> flashrom would not allow that due to a size mismatch). flashrom would not
> indicate the problem in any output - so far we only check the return value
> of fwrite() that is not conclusive.
>
> This patch checks the return values of all related system calls like fclose()
> unless we only read the file and are not really interested in output errors.
> In the latter case the return value is casted to void to document this fact.
> Additionally, this patch explicitly calls fflush() and fsync() (on regular files only)
> to do the best we can to guarantee the read image reaches the disk safely.
>
> Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> ---
>  flashrom.c         | 67 +++++++++++++++++++++++++++++++++++++-----------------
>  layout.c           |  6 ++---
>  processor_enable.c |  4 ++--
>  3 files changed, 51 insertions(+), 26 deletions(-)
>
> diff --git a/flashrom.c b/flashrom.c
> index c9c7e31..6e5fd68 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -28,6 +28,7 @@
>  #include <sys/stat.h>
>  #endif
>  #include <string.h>
> +#include <unistd.h>
>  #include <stdlib.h>
>  #include <errno.h>
>  #include <ctype.h>
> @@ -1255,36 +1256,39 @@ int read_buf_from_file(unsigned char *buf, unsigned long size,
>         msg_gerr("Error: No file I/O support in libpayload\n");
>         return 1;
>  #else
> -       unsigned long numbytes;
> -       FILE *image;
> -       struct stat image_stat;
> +       int ret = 0;
>
> +       FILE *image;
>         if ((image = fopen(filename, "rb")) == NULL) {
>                 msg_gerr("Error: opening file \"%s\" failed: %s\n", filename, strerror(errno));
>                 return 1;
>         }
> +
> +       struct stat image_stat;
>         if (fstat(fileno(image), &image_stat) != 0) {
>                 msg_gerr("Error: getting metadata of file \"%s\" failed: %s\n", filename, strerror(errno));
> -               fclose(image);
> -               return 1;
> +               ret = 1;
> +               goto out;
>         }
>         if (image_stat.st_size != size) {
>                 msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n",
>                          (intmax_t)image_stat.st_size, size);
> -               fclose(image);
> -               return 1;
> -       }
> -       numbytes = fread(buf, 1, size, image);
> -       if (fclose(image)) {
> -               msg_gerr("Error: closing file \"%s\" failed: %s\n", filename, strerror(errno));
> -               return 1;
> +               ret = 1;
> +               goto out;
>         }
> +
> +       unsigned long numbytes = fread(buf, 1, size, image);
>         if (numbytes != size) {
>                 msg_gerr("Error: Failed to read complete file. Got %ld bytes, "
>                          "wanted %ld!\n", numbytes, size);
> -               return 1;
> +               ret = 1;
>         }
> -       return 0;
> +out:
> +       if (fclose(image)) {
> +               msg_gerr("Error: closing file \"%s\" failed: %s\n", filename, strerror(errno));
> +               ret = 1;
> +       }
> +       return ret;
>  #endif
>  }
Why do this goto stuff and fclose fail reporting to read_buf_from_file?
I'd suggest same handling as below for layout files etc (= (void)fclose()).


>
> @@ -1294,8 +1298,8 @@ int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *
>         msg_gerr("Error: No file I/O support in libpayload\n");
>         return 1;
>  #else
> -       unsigned long numbytes;
>         FILE *image;
> +       int ret = 0;
>
>         if (!filename) {
>                 msg_gerr("No filename specified.\n");
> @@ -1306,14 +1310,35 @@ int write_buf_to_file(const unsigned char *buf, unsigned long size, const char *
>                 return 1;
>         }
>
> -       numbytes = fwrite(buf, 1, size, image);
> -       fclose(image);
> +       unsigned long numbytes = fwrite(buf, 1, size, image);
>         if (numbytes != size) {
> -               msg_gerr("File %s could not be written completely.\n",
> -                        filename);
> -               return 1;
> +               msg_gerr("Error: file %s could not be written completely.\n", filename);
> +               ret = 1;
> +               goto out;
>         }
> -       return 0;
> +       if (fflush(image)) {
> +               msg_gerr("Error: flushing file \"%s\" failed: %s\n", filename, strerror(errno));
> +               ret = 1;
> +       }
> +       struct stat image_stat;
> +       if (fstat(fileno(image), &image_stat) != 0) {
> +               msg_gerr("Error: getting metadata of file \"%s\" failed: %s\n", filename, strerror(errno));
> +               ret = 1;
> +               goto out;
> +       }
> +       /* Try to fsync only regular files. */
> +       if (S_ISREG(image_stat.st_mode)) {
> +               if (fsync(fileno(image))) {
> +                       msg_gerr("Error: fsyncing file \"%s\" failed: %s\n", filename, strerror(errno));
> +                       ret = 1;
> +               }
> +       }
> +out:
> +       if (fclose(image)) {
> +               msg_gerr("Error: closing file \"%s\" failed: %s\n", filename, strerror(errno));
> +               ret = 1;
> +       }
> +       return ret;
>  #endif
>  }
>
> diff --git a/layout.c b/layout.c
> index 0b9f6e5..d039451 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -65,7 +65,7 @@ int read_romlayout(const char *name)
>                 if (num_rom_entries >= MAX_ROMLAYOUT) {
>                         msg_gerr("Maximum number of ROM images (%i) in layout "
>                                  "file reached.\n", MAX_ROMLAYOUT);
> -                       fclose(romlayout);
> +                       (void)fclose(romlayout);
>                         return 1;
>                 }
>                 if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[num_rom_entries].name))
> @@ -80,7 +80,7 @@ int read_romlayout(const char *name)
>                 tstr2 = strtok(NULL, ":");
>                 if (!tstr1 || !tstr2) {
>                         msg_gerr("Error parsing layout file. Offending string: \"%s\"\n", tempstr);
> -                       fclose(romlayout);
> +                       (void)fclose(romlayout);
>                         return 1;
>                 }
>                 rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16);
> @@ -95,7 +95,7 @@ int read_romlayout(const char *name)
>                              rom_entries[i].end, rom_entries[i].name);
>         }
>
> -       fclose(romlayout);
> +       (void)fclose(romlayout);
>
>         return 0;
>  }
> diff --git a/processor_enable.c b/processor_enable.c
> index 1361dd5..117aa1e 100644
> --- a/processor_enable.c
> +++ b/processor_enable.c
> @@ -57,11 +57,11 @@ static int is_loongson(void)
>                 ptr++;
>                 while (*ptr && isspace((unsigned char)*ptr))
>                         ptr++;
> -               fclose(cpuinfo);
> +               (void)fclose(cpuinfo);
>                 return (strncmp(ptr, "ICT Loongson-2 V0.3", strlen("ICT Loongson-2 V0.3")) == 0) ||
>                        (strncmp(ptr, "Godson2 V0.3  FPU V0.1", strlen("Godson2 V0.3  FPU V0.1")) == 0);
>         }
> -       fclose(cpuinfo);
> +       (void)fclose(cpuinfo);
>         return 0;
>  }
>  #endif
> --
> Kind regards, Stefan Tauner
>
>
> _______________________________________________
> flashrom mailing list
> flashrom at flashrom.org
> http://www.flashrom.org/mailman/listinfo/flashrom

If you fix that (or convince me that you want to print reading fclose
failure), this is acked by me.

-- 
Urja Rannikko




More information about the flashrom mailing list