[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