Change in flashrom[master]: it85spi.c: Kill off dead code system() hack

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

it85spi.c: Kill off dead code system() hack

This is dead code sitting under a 'if 0' and is
extremely hacky. One could argue this patch perhaps
should include a update to documentation to stop
powerd instead of keeping this rot around?

BUG=none
BRANCH=none
TEST=builds

Change-Id: I5761dea48d553b1334eef3c9cf0ad79cedbd7806
Signed-off-by: Edward O'Callaghan 
---
M it85spi.c
1 file changed, 0 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/47/46547/1

diff --git a/it85spi.c b/it85spi.c
index b998790..1d93a5b 100644
--- a/it85spi.c
+++ b/it85spi.c
@@ -114,15 +114,6 @@
    if (it85xx_scratch_rom_reenter > 0)
    	return;

-#if 0
-	/* FIXME: this a workaround for the bug that SMBus signal would
-	 *        interfere the EC firmware update. Should be removed if
-	 *        we find out the root cause. */
-	ret = system("stop powerd >&2");
-	if (ret)
-		msg_perr("Cannot stop powerd.\n");
-#endif
-
    for (tries = 0; tries < MAX_TRY; ++tries) {
    	/* Wait until IBF (input buffer) is not full. */
    	if (wait_for(KB_IBF, 0, MAX_TIMEOUT,
@@ -165,9 +156,6 @@

static void it85xx_exit_scratch_rom(void)
 {
-#if 0
-	int ret;
-#endif
    int tries;

msg_pdbg("%s():%d was called ...\n", __func__, __LINE__);
@@ -204,15 +192,6 @@
    } else {
    	msg_perr("%s():%d * Max try reached.\n", __func__, __LINE__);
    }
-
-#if 0
-	/* FIXME: this a workaround for the bug that SMBus signal would
-	 *        interfere the EC firmware update. Should be removed if
-	 *        we find out the root cause. */
-	ret = system("start powerd >&2");
-	if (ret)
-		msg_perr("Cannot start powerd again.\n");
-#endif
 }

static int it85xx_shutdown(void *data)

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1: Code-Review-1

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1:

I once tried it85spi on an ITE IT8502E EC, just probing made the laptop power off after a few seconds.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1:
> Patch Set 1:
>
> I once tried it85spi on an ITE IT8502E EC, just probing made the laptop power off after a few seconds.

eek yea ok. Thanks for the extra info we should perhaps try to fix it once I can recover time from Chromium deforking. Sounds like we should put a warning in the man page?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> >
> > I once tried it85spi on an ITE IT8502E EC, just probing made the laptop power off after a few seconds.
>
> eek yea ok. Thanks for the extra info we should perhaps try to fix it once I can recover time from Chromium deforking. Sounds like we should put a warning in the man page?

The code doesn't run unless one uncomments a function call somewhere.

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1: Code-Review+2

While the driver is called IT8502E, it is talking to a very specific version of the EC firmware that was running on a very specific set of machines that used to have this chip.

Chances are that none of these machines are in operation anymore, and if they are, they are most likely no longer relevant. But that goes for a lot of the code in coreboot / flashrom et all.

Angel, chances are also, that your behavior is caused by one of two things happening:

1. the CPU is catching IO writes to the IO ports in question and, because flashrom does not pass the consistency checks for these accesses, an SMI reboots the machine.

2. the IT8502 firmware on your machine gets a command that it does not understand, because it's not running the firmware from the same ODM as the IT8502 that this driver was developed against, and the EC decides that because someone is sending potentially evil commands, the EC pulls the AP's reset line to stop the behavior. In a lot of these older systems there is no actual security strategy in the software design, and so the only way to keep the device safe is by obscurity. To prevent people from reverse engineering the project, rate limiting the access through resets is a somewhat reasonable method (hu hu).

I bet that we could remove the it85spi driver from flashrom all together and no person or entity would actually ever care. Of course we might want to decide against it for educational reasons.

[Stefan Reinauer (Code Review)]

Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1: -Code-Review

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/46547 )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review+2
>
> While the driver is called IT8502E, it is talking to a very specific version of the EC firmware that was running on a very specific set of machines that used to have this chip.
>
> Chances are that none of these machines are in operation anymore, and if they are, they are most likely no longer relevant. But that goes for a lot of the code in coreboot / flashrom et all.
>
> Angel, chances are also, that your behavior is caused by one of two things happening:
> the CPU is catching IO writes to the IO ports in question and, because flashrom does not pass the consistency checks for these accesses, an SMI reboots the machine.

It doesn't reboot the machine, it just powers off.
> the IT8502 firmware on your machine gets a command that it does not understand, because it's not running the firmware from the same ODM as the IT8502 that this driver was developed against, and the EC decides that because someone is sending potentially evil commands, the EC pulls the AP's reset line to stop the behavior. In a lot of these older systems there is no actual security strategy in the software design, and so the only way to keep the device safe is by obscurity. To prevent people from reverse engineering the project, rate limiting the access through resets is a somewhat reasonable method (hu hu).

I doubt the firmware on that device has any consideration w.r.t. security. I still need to look at it more closely, but I'm getting buried under more and more computers.
> I bet that we could remove the it85spi driver from flashrom all together and no person or entity would actually ever care. Of course we might want to decide against it for educational reasons.

If anything, we would need to ensure we're only trying to use the code on whitelisted machines. Currently, there's no whitelist and the call to the it85spi code is commented-out, sooo... it doesn't make a difference. It's still being build-tested, though, so it's not as rotten as it could be.

[S] Change in flashrom[main]: it85spi.c: Kill off dead code system() hack

[Anastasia Klimchuk (Code Review)]

Anastasia Klimchuk has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/46547?usp=email )

Change subject: it85spi.c: Kill off dead code system() hack
......................................................................

Abandoned

it85spi has been deleted