From 70838cc1a1294f1f53d00c08d3ee7616db073e8e Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Thu, 8 Apr 2021 11:46:26 +0800 Subject: cgpt: Use subprocess_run to call flashrom with 1>/dev/null Closing fd=1 is a bad idea, as flashrom will then reuse fd=1 for the MTD device and directly write to it. Luckily, we have a subprocess_run function in vboot that does what we need. There will be some cleanup required after this, but hopefully this is enough to clear the P0 on hand. BUG=b:184559695 TEST=`cgpt find -t kernel` (fails?!), but at least does not corrupt flash: `flashrom -r /usr/local/x.bin && hexdump -C /usr/local/x.bin | head` BRANCH=none Change-Id: Ia82ed7966ea66274f72fe21eca5241633ffbdb5c Signed-off-by: Nicolas Boichat Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/vboot_reference/+/2812630 Reviewed-by: Mike Frysinger Reviewed-by: Jack Rosenthal Commit-Queue: Jack Rosenthal --- cgpt/cgpt_nor.c | 75 ++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 19 deletions(-) diff --git a/cgpt/cgpt_nor.c b/cgpt/cgpt_nor.c index fd184463..40306676 100644 --- a/cgpt/cgpt_nor.c +++ b/cgpt/cgpt_nor.c @@ -24,6 +24,7 @@ #include "cgpt.h" #include "cgpt_nor.h" +#include "subprocess.h" static const char FLASHROM_PATH[] = "/usr/sbin/flashrom"; @@ -48,6 +49,7 @@ int GetMtdSize(const char *mtd_device, uint64_t *size) { return ret; } +// TODO(b:184559695): Remove these functions and use subprocess_run everywhere. int ForkExecV(const char *cwd, const char *const argv[]) { pid_t pid = fork(); if (pid == -1) { @@ -212,27 +214,42 @@ int ReadNorFlash(char *temp_dir_template) { // Read RW_GPT section from NOR flash to "rw_gpt". ret++; - int fd_flags = fcntl(1, F_GETFD); - // Close stdout on exec so that flashrom does not muck up cgpt's output. - if (0 != fcntl(1, F_SETFD, FD_CLOEXEC)) - Warning("Can't stop flashrom from mucking up our output\n"); - if (ForkExecL(temp_dir_template, FLASHROM_PATH, "-i", "RW_GPT:rw_gpt", "-r", - NULL) != 0) { + + // TODO(b:184559695): Add parameter to subprocess_run to change directory + // before exec. Also, NULL parameter is a glibc extension that _might_ + // break FreeBSD. + char *cwd = getcwd(NULL, 0); + if (!cwd) { + Error("Cannot get current directory.\n"); + return ret; + } + if (chdir(temp_dir_template) < 0) { + Error("Cannot change directory.\n"); + goto out_free; + } + const char *const argv[] = {FLASHROM_PATH, "-i", "RW_GPT:rw_gpt", "-r"}; + // Redirect stdout to /dev/null so that flashrom does not muck up cgpt's + // output. + if (subprocess_run(argv, &subprocess_null, &subprocess_null, NULL) != 0) { Error("Cannot exec flashrom to read from RW_GPT section.\n"); RemoveDir(temp_dir_template); } else { ret = 0; } + if (chdir(cwd) < 0) { + Error("Cannot change directory back to original.\n"); + goto out_free; + } - // Restore stdout flags - if (0 != fcntl(1, F_SETFD, fd_flags)) - Warning("Can't restore stdout flags\n"); +out_free: + free(cwd); return ret; } // Write "rw_gpt" back to NOR flash. We write the file in two parts for safety. int WriteNorFlash(const char *dir) { int ret = 0; + ret++; if (split_gpt(dir, "rw_gpt") != 0) { Error("Cannot split rw_gpt in two.\n"); @@ -240,26 +257,46 @@ int WriteNorFlash(const char *dir) { } ret++; int nr_fails = 0; - int fd_flags = fcntl(1, F_GETFD); - // Close stdout on exec so that flashrom does not muck up cgpt's output. - if (0 != fcntl(1, F_SETFD, FD_CLOEXEC)) - Warning("Can't stop flashrom from mucking up our output\n"); - if (ForkExecL(dir, FLASHROM_PATH, "-i", "RW_GPT_PRIMARY:rw_gpt_1", - "-w", "--fast-verify", NULL) != 0) { + + // TODO(b:184559695): Add parameter to subprocess_run to change directory + // before exec. Also, NULL parameter is a glibc extension that _might_ + // break FreeBSD. + char *cwd = getcwd(NULL, 0); + if (!cwd) { + Error("Cannot get current directory.\n"); + return ret; + } + if (chdir(dir) < 0) { + Error("Cannot change directory.\n"); + goto out_free; + } + const char *const argv1[] = {FLASHROM_PATH, "-i", "RW_GPT_PRIMARY:rw_gpt_1", + "-w", "--fast-verify"}; + // Redirect stdout to /dev/null so that flashrom does not muck up cgpt's + // output. + if (subprocess_run(argv1, &subprocess_null, &subprocess_null, NULL) != 0) { Warning("Cannot write the 1st half of rw_gpt back with flashrom.\n"); nr_fails++; } - if (ForkExecL(dir, FLASHROM_PATH, "-i", "RW_GPT_SECONDARY:rw_gpt_2", - "-w", "--fast-verify", NULL) != 0) { + const char *const argv2[] = {FLASHROM_PATH, "-i", "RW_GPT_SECONDARY:rw_gpt_2", + "-w", "--fast-verify"}; + // Redirect stdout to /dev/null so that flashrom does not muck up cgpt's + // output. + if (subprocess_run(argv2, &subprocess_null, &subprocess_null, NULL) != 0) { Warning("Cannot write the 2nd half of rw_gpt back with flashrom.\n"); nr_fails++; } - if (0 != fcntl(1, F_SETFD, fd_flags)) - Warning("Can't restore stdout flags\n"); + if (chdir(cwd) < 0) { + Error("Cannot change directory back to original.\n"); + goto out_free; + } switch (nr_fails) { case 0: ret = 0; break; case 1: Warning("It might still be okay.\n"); break; case 2: Error("Cannot write both parts back with flashrom.\n"); break; } + +out_free: + free(cwd); return ret; } -- cgit v1.2.1