Change in flashrom[staging]: rpci: Use pci_dev struct pointer to avoid API breaks

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/20784 )

Change subject: rpci: Use pci_dev struct pointer to avoid API breaks
......................................................................

rpci: Use pci_dev struct pointer to avoid API breaks

The pci_dev structure is never meant to be used as is, but always as a
pointer. By using the struct itself in undo_pci_write_data, we are risking
data corruption, or buffer overflows if the structure size changes.

This is especially apparent on my system where flashrom segfaults
because I compile it with pciutils 3.3.0 and I run it on a system
with pciutils 3.5.2. The struture size is different and causes a
struct with the wrong size to be sent to the library, with invalid
internal field values.

This has been discovered and discussed in Change ID 18925 [1]

[1] https://review.coreboot.org/#/c/18925/

Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298
Signed-off-by: Youness Alaoui <[e-mail]>
Reviewed-on: https://review.coreboot.org/20784
Tested-by: build bot (Jenkins) <[e-mail]>
Reviewed-by: Nico Huber <[e-mail]>
---
M chipset_enable.c
M pcidev.c
M programmer.h
3 files changed, 25 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved



diff --git a/chipset_enable.c b/chipset_enable.c
index 20d2662..6a93d0d 100644
--- a/chipset_enable.c
+++ b/chipset_enable.c
@@ -843,6 +843,7 @@
 	 * straints (e.g. on PCI domains, extended PCIe config space).
 	 */
 	struct pci_access *const pci_acc = pci_alloc();
+	struct pci_access *const saved_pacc = pacc;
 	if (!pci_acc) {
 		msg_perr("Can't allocate PCI accessor.\n");
 		return ret;
@@ -857,6 +858,9 @@
 		return ret;
 	}
 
+	/* Modify pacc so the rpci_write can register the undo callback with a
+	 * device using the correct pci_access */
+	pacc = pci_acc;
 	enable_flash_ich_report_gcs(spi_dev, pch_generation, NULL);
 
 	const int ret_bc = enable_flash_ich_bios_cntl_config_space(spi_dev, pch_generation, 0xdc);
@@ -880,6 +884,7 @@
 
 _freepci_ret:
 	pci_free_dev(spi_dev);
+	pacc = saved_pacc;
 	return ret;
 }
 
diff --git a/pcidev.c b/pcidev.c
index 2c78063..f4e5542 100644
--- a/pcidev.c
+++ b/pcidev.c
@@ -160,6 +160,7 @@
 		return 1;
 	}
 	pci_cleanup(pacc);
+	pacc = NULL;
 	return 0;
 }
 
@@ -259,7 +260,7 @@
 };
 
 struct undo_pci_write_data {
-	struct pci_dev dev;
+	struct pci_dev *dev;
 	int reg;
 	enum pci_write_type type;
 	union {
@@ -272,22 +273,23 @@
 int undo_pci_write(void *p)
 {
 	struct undo_pci_write_data *data = p;
-	if (pacc == NULL) {
-		msg_perr("%s: Tried to undo PCI writes without a valid PCI context!\n"
-			 "Please report a bug at [e-mail]\n", __func__);
+	if (pacc == NULL || data->dev == NULL) {
+		msg_perr("%s: Tried to undo PCI writes without a valid PCI %s!\n"
+			"Please report a bug at [e-mail]\n",
+			__func__, data->dev == NULL ? "device" : "context");
 		return 1;
 	}
 	msg_pdbg("Restoring PCI config space for %02x:%02x:%01x reg 0x%02x\n",
-		 data->dev.bus, data->dev.dev, data->dev.func, data->reg);
+		 data->dev->bus, data->dev->dev, data->dev->func, data->reg);
 	switch (data->type) {
 	case pci_write_type_byte:
-		pci_write_byte(&data->dev, data->reg, data->bytedata);
+		pci_write_byte(data->dev, data->reg, data->bytedata);
 		break;
 	case pci_write_type_word:
-		pci_write_word(&data->dev, data->reg, data->worddata);
+		pci_write_word(data->dev, data->reg, data->worddata);
 		break;
 	case pci_write_type_long:
-		pci_write_long(&data->dev, data->reg, data->longdata);
+		pci_write_long(data->dev, data->reg, data->longdata);
 		break;
 	}
 	/* p was allocated in register_undo_pci_write. */
@@ -303,7 +305,11 @@
 		msg_gerr("Out of memory!\n");				\
 		exit(1);						\
 	}								\
-	undo_pci_write_data->dev = *a;					\
+	if (pacc)							\
+		undo_pci_write_data->dev = pci_get_dev(pacc,		\
+				a->domain, a->bus, a->dev, a->func);	\
+	else								\
+		undo_pci_write_data->dev =  NULL;			\
 	undo_pci_write_data->reg = b;					\
 	undo_pci_write_data->type = pci_write_type_##c;			\
 	undo_pci_write_data->c##data = pci_read_##c(dev, reg);		\
diff --git a/programmer.h b/programmer.h
index ec00bd9..e58fd32 100644
--- a/programmer.h
+++ b/programmer.h
@@ -195,6 +195,11 @@
 struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar);
 /* rpci_write_* are reversible writes. The original PCI config space register
  * contents will be restored on shutdown.
+ * To clone the pci_dev instances internally, the `pacc` global
+ * variable has to reference a pci_access method that is compatible
+ * with the given pci_dev handle. The referenced pci_access (not
+ * the variable) has to stay valid until the shutdown handlers are
+ * finished.
  */
 int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data);
 int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data);

-- 
To view, visit https://review.coreboot.org/20784
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-Project: flashrom
Gerrit-Branch: staging
Gerrit-MessageType: merged
Gerrit-Change-Id: Icde2e587992ba964d4ff92c33aa659850ba06298
Gerrit-Change-Number: 20784
Gerrit-PatchSet: 5
Gerrit-Owner: Youness Alaoui <[e-mail]>
Gerrit-Reviewer: David Hendricks <[e-mail]>
Gerrit-Reviewer: Nico Huber <[e-mail]>
Gerrit-Reviewer: Paul Menzel <[e-mail]>
Gerrit-Reviewer: Youness Alaoui <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>