[coreboot-gerrit] Change in ...coreboot[master]: sb/intel/lynxpoint/pcie.c: Add more checks for NULL pointers

[Tristan Corrick (Code Review)]

Tristan Corrick has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30079


Change subject: sb/intel/lynxpoint/pcie.c: Add more checks for NULL pointers
......................................................................

sb/intel/lynxpoint/pcie.c: Add more checks for NULL pointers

If PCIe root port `n` is disabled, then `rpc.ports[n - 1]` remains NULL.
The existing Lynx Point systems probably don't end up dereferencing
NULL pointers this way. However, it might occur on a system using
Flexible I/O to remap PCIe root ports to other functions.

Tested on an ASRock H81M-HDS and an Acer C720 (Google Peppy). No issues
presented themselves.

Change-Id: I2c22fa36217766c2c4d6e8046f99989063066b16
Signed-off-by: Tristan Corrick <tristan at corrick.kiwi>
---
M src/southbridge/intel/lynxpoint/pcie.c
1 file changed, 29 insertions(+), 16 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/79/30079/1

diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c
index 231f3e7..695abf2 100644
--- a/src/southbridge/intel/lynxpoint/pcie.c
+++ b/src/southbridge/intel/lynxpoint/pcie.c
@@ -14,6 +14,8 @@
  * GNU General Public License for more details.
  */
 
+#include <assert.h>
+#include <commonlib/helpers.h>
 #include <console/console.h>
 #include <device/device.h>
 #include <device/pci.h>
@@ -22,6 +24,8 @@
 #include <device/pci_ops.h>
 #include "pch.h"
 #include <southbridge/intel/common/gpio.h>
+#include <stddef.h>
+#include <stdint.h>
 
 #define MAX_NUM_ROOT_PORTS 8
 
@@ -68,6 +72,16 @@
 	return PCI_FUNC(dev->path.pci.devfn) + 1;
 }
 
+static bool is_rp_enabled(int rp)
+{
+	ASSERT(rp > 0 && rp <= ARRAY_SIZE(rpc.ports));
+
+	if (rpc.ports[rp - 1] == NULL)
+		return false;
+
+	return rpc.ports[rp - 1]->enabled;
+}
+
 static void root_port_config_update_gbe_port(void)
 {
 	/* Is the Gbe Port enabled? */
@@ -163,7 +177,7 @@
 	/* Determine the new devfn for this port */
 	new_devfn = PCI_DEVFN(PCH_PCIE_DEV_SLOT, pci_func);
 
-	if (dev->path.pci.devfn != new_devfn) {
+	if (dev && dev->path.pci.devfn != new_devfn) {
 		printk(BIOS_DEBUG,
 		       "PCH: PCIe map %02x.%1x -> %02x.%1x\n",
 		       PCI_SLOT(dev->path.pci.devfn),
@@ -188,9 +202,12 @@
 		int rp;
 
 		dev = rpc.ports[i];
+		if (!dev)
+			continue;
+
 		rp = root_port_number(dev);
 
-		if (!dev->enabled) {
+		if (!is_rp_enabled(rp)) {
 			static const uint32_t high_bit = (1UL << 31);
 
 			/* Configure shared resource clock gating. */
@@ -198,17 +215,13 @@
 				pci_update_config8(dev, 0xe1, 0xc3, 0x3c);
 
 			if (!is_lp) {
-				if (rp == 1 && !rpc.ports[1]->enabled &&
-				    !rpc.ports[2]->enabled &&
-				    !rpc.ports[3]->enabled) {
+				if (rp == 1 && !is_rp_enabled(2) &&
+				    !is_rp_enabled(3) && !is_rp_enabled(4)) {
 					pci_update_config8(dev, 0xe2, ~1, 1);
 					pci_update_config8(dev, 0xe1, 0x7f, 0x80);
 				}
-				if (rp == 5 && !rpc.ports[5]->enabled &&
-				    (rpc.ports[6] == NULL ||
-						!rpc.ports[6]->enabled) &&
-				    (rpc.ports[7] == NULL ||
-						!rpc.ports[7]->enabled)) {
+				if (rp == 5 && !is_rp_enabled(6) &&
+				    !is_rp_enabled(7) && !is_rp_enabled(8)) {
 					pci_update_config8(dev, 0xe2, ~1, 1);
 					pci_update_config8(dev, 0xe1, 0x7f, 0x80);
 				}
@@ -223,8 +236,8 @@
 				pci_update_config32(dev, 0x420, ~0, (3 << 29));
 
 			/* Enable static clock gating. */
-			if (rp == 1 && !rpc.ports[1]->enabled &&
-			    !rpc.ports[2]->enabled && !rpc.ports[3]->enabled) {
+			if (rp == 1 && !is_rp_enabled(2) &&
+			    !is_rp_enabled(3) && !is_rp_enabled(4)) {
 				pci_update_config8(dev, 0xe2, ~1, 1);
 				pci_update_config8(dev, 0xe1, 0x7f, 0x80);
 			} else if (rp == 5 || rp == 6) {
@@ -258,7 +271,7 @@
 			pci_update_config8(dev, 0xe1, 0xc3, 0x3c);
 	}
 
-	if (!enabled_ports && is_lp)
+	if (!enabled_ports && is_lp && rpc.ports[0])
 		pci_update_config8(rpc.ports[0], 0xe1, ~(1 << 6), (1 << 6));
 }
 
@@ -267,7 +280,7 @@
 	int i;
 
 	/* If the first root port is disabled the coalesce ports. */
-	if (!rpc.ports[0]->enabled)
+	if (!is_rp_enabled(1))
 		rpc.coalesce = 1;
 
 	/* Perform clock gating configuration. */
@@ -307,7 +320,7 @@
 		 * function numbers. */
 		current_func = 0;
 		for (i = 0; i < rpc.num_ports; i++) {
-			if (!rpc.ports[i]->enabled)
+			if (!is_rp_enabled(i + 1))
 				continue;
 			pch_pcie_device_set_func(i, current_func);
 			current_func++;
@@ -315,7 +328,7 @@
 
 		/* Allocate the disabled devices' PCI function number. */
 		for (i = 0; i < rpc.num_ports; i++) {
-			if (rpc.ports[i]->enabled)
+			if (is_rp_enabled(i + 1))
 				continue;
 			pch_pcie_device_set_func(i, current_func);
 			current_func++;

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/30079
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I2c22fa36217766c2c4d6e8046f99989063066b16
Gerrit-Change-Number: 30079
Gerrit-PatchSet: 1
Gerrit-Owner: Tristan Corrick <tristan at corrick.kiwi>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181206/600c8fe0/attachment.html>