Change in ...coreboot[master]: device/pci_device.c: Use verified boot to check oprom

[Frans Hendriks (Code Review)]

Frans Hendriks has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30810

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

device/pci_device.c: Use verified boot to check oprom

Before oprom is executed, no check is performed if rom passes verification.
Add call to verified_boot_should_run_oprom() to verify the oprom.

BUG=N/A
TEST=Created verified binary and verify logging on Portwell PQ-M107

Change-Id: Iec5092e85d34940ea3a3bb1192ea49f3bc3e5b27
Signed-off-by: Frans Hendriks 
---
M src/device/pci_device.c
M src/include/device/pci_rom.h
2 files changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/30810/1

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 82033a6..40012f9 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -16,6 +16,7 @@
  * Copyright (C) 2005-2009 coresystems GmbH
  * (Written by Stefan Reinauer  for coresystems GmbH)
  * Copyright (C) 2014 Sage Electronic Engineering, LLC.
+ * Copyright (C) 2018 Eltan B.V.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -802,6 +803,10 @@
    if (!should_run_oprom(dev))
    	return;

+	if (IS_ENABLED(CONFIG_VERIFIED_BOOT))
+		if (!verified_boot_should_run_oprom(rom))
+			return;
+
    run_bios(dev, (unsigned long)ram);
    gfx_set_init_done(1);
    printk(BIOS_DEBUG, "VGA Option ROM was run\n");
diff --git a/src/include/device/pci_rom.h b/src/include/device/pci_rom.h
index a4aa52a..865fea3 100644
--- a/src/include/device/pci_rom.h
+++ b/src/include/device/pci_rom.h
@@ -1,3 +1,19 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2015 Google Inc.
+ * Copyright (C) 2018 Eltan B.V.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
 #ifndef PCI_ROM_H
 #define PCI_ROM_H
 #include <endian.h>
@@ -47,4 +63,5 @@

u32 map_oprom_vendev(u32 vendev);

+int verified_boot_should_run_oprom(struct rom_header *rom_header);
 #endif

[Angel Pons (Code Review)]

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

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 1:

I guess this doesn't compile because it needs stuff from another of your patches? See my comment on CB:30218

[Stefan Reinauer (Code Review)]

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

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 1: Code-Review+1

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 3:

Can someone review this patch?

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/30810/3/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/#/c/30810/3/src/device/pci_device.c@806 
PS3, Line 806: (IS_ENABLED(CONFIG_VERIFIED_BOOT))
CONFIG(

https://review.coreboot.org/#/c/30810/3/src/device/pci_device.c@807 
PS3, Line 807: verified_boot_should_run_oprom
Move to should_run_oprom

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/30810/3/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/#/c/30810/3/src/device/pci_device.c@806 
PS3, Line 806: (IS_ENABLED(CONFIG_VERIFIED_BOOT))
> CONFIG(

Done

https://review.coreboot.org/#/c/30810/3/src/device/pci_device.c@807 
PS3, Line 807: verified_boot_should_run_oprom
> Move to should_run_oprom

Done

[Frans Hendriks (Code Review)]

Hello Angel Pons, Stefan Reinauer, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/30810

to look at the new patch set (#4).

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

device/pci_device.c: Use verified boot to check oprom

Before oprom is executed, no check is performed if rom passes verification.
Add call to verified_boot_should_run_oprom() to verify the oprom.

verified_boot_should_run_oprom() expects and rom address as input pointer.
*rom is added as input parameter to should_run_oprom() which must be parsed
to verified_boot_should_run_oprom()..

BUG=N/A
TEST=Created verified binary and verify logging on Facebook FBG1701

Change-Id: Iec5092e85d34940ea3a3bb1192ea49f3bc3e5b27
Signed-off-by: Frans Hendriks 
---
M src/device/pci_device.c
1 file changed, 9 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/30810/4

[Frans Hendriks (Code Review)]

Hello Angel Pons, Stefan Reinauer, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/30810

to look at the new patch set (#6).

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

device/pci_device.c: Use verified boot to check oprom

Before oprom is executed, no check is performed if rom passes verification.
Add call to verified_boot_should_run_oprom() to verify the oprom.

verified_boot_should_run_oprom() expects and rom address as input pointer.
*rom is added as input parameter to should_run_oprom() which must be parsed
to verified_boot_should_run_oprom()..

BUG=N/A
TEST=Created verified binary and verify logging on Facebook FBG1701

Change-Id: Iec5092e85d34940ea3a3bb1192ea49f3bc3e5b27
Signed-off-by: Frans Hendriks 
---
M src/device/pci_device.c
M src/include/device/pci_rom.h
2 files changed, 26 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/30810/6

Change in coreboot[master]: device/pci_device.c: Use verified boot to check oprom

[Lance Zhao (Code Review)]

Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 7: Code-Review+1

Change in coreboot[master]: device/pci_device.c: Use verified boot to check oprom

[Frans Hendriks (Code Review)]

Hello Patrick Rudolph, Angel Pons, Stefan Reinauer, Lance Zhao, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/30810

to look at the new patch set (#8).

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

device/pci_device.c: Use verified boot to check oprom

Before oprom is executed, no check is performed if rom passes verification.
Add call to verified_boot_should_run_oprom() to verify the oprom.

verified_boot_should_run_oprom() expects and rom address as input pointer.
*rom is added as input parameter to should_run_oprom() which must be parsed
to verified_boot_should_run_oprom()..

BUG=N/A
TEST=Created verified binary and verify logging on Facebook FBG1701

Change-Id: Iec5092e85d34940ea3a3bb1192ea49f3bc3e5b27
Signed-off-by: Frans Hendriks 
---
M src/device/pci_device.c
M src/include/device/pci_rom.h
2 files changed, 9 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/30810/8

Change in coreboot[master]: device/pci_device.c: Use verified boot to check oprom

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 9: Code-Review+2

Change in coreboot[master]: device/pci_device.c: Use verified boot to check oprom

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

device/pci_device.c: Use verified boot to check oprom

Before oprom is executed, no check is performed if rom passes verification.
Add call to verified_boot_should_run_oprom() to verify the oprom.

verified_boot_should_run_oprom() expects and rom address as input pointer.
*rom is added as input parameter to should_run_oprom() which must be parsed
to verified_boot_should_run_oprom()..

BUG=N/A
TEST=Created verified binary and verify logging on Facebook FBG1701

Change-Id: Iec5092e85d34940ea3a3bb1192ea49f3bc3e5b27
Signed-off-by: Frans Hendriks 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/30810
Tested-by: build bot (Jenkins) 
Reviewed-by: Philipp Deppenwiese 
---
M src/device/pci_device.c
M src/include/device/pci_rom.h
2 files changed, 9 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved

diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index c043dd6..0a4b69b 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -679,10 +679,15 @@
    }
 }

-static int should_run_oprom(struct device *dev)
+static int should_run_oprom(struct device *dev, struct rom_header *rom)
 {
    static int should_run = -1;

+	if (CONFIG(VENDORCODE_ELTAN_VBOOT))
+		if (rom != NULL)
+			if (!verified_boot_should_run_oprom(rom))
+				return 0;
+
    if (should_run >= 0)
    	return should_run;

@@ -711,7 +716,7 @@
    	return 0;
    if (CONFIG(ALWAYS_LOAD_OPROM))
    	return 1;
-	if (should_run_oprom(dev))
+	if (should_run_oprom(dev, NULL))
    	return 1;

return 0;
@@ -742,7 +747,7 @@
    	return;
    timestamp_add_now(TS_OPROM_COPY_END);

-	if (!should_run_oprom(dev))
+	if (!should_run_oprom(dev, rom))
    	return;

run_bios(dev, (unsigned long)ram);
diff --git a/src/include/device/pci_rom.h b/src/include/device/pci_rom.h
index a4aa52a..82f3c40 100644
--- a/src/include/device/pci_rom.h
+++ b/src/include/device/pci_rom.h
@@ -47,4 +47,5 @@

u32 map_oprom_vendev(u32 vendev);

+int verified_boot_should_run_oprom(struct rom_header *rom_header);
 #endif

Change in coreboot[master]: device/pci_device.c: Use verified boot to check oprom

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30810 )

Change subject: device/pci_device.c: Use verified boot to check oprom
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/30810/10/src/device/pci_device.c 
File src/device/pci_device.c:

https://review.coreboot.org/c/coreboot/+/30810/10/src/device/pci_device.c@75... 
PS10, Line 750: 	if (!should_run_oprom(dev, rom))
Why is `rom` checked and not the `ram` copy? And if there is a reason
to accept the TOCTOU weakness, why load it if it doesn't verify?