Change in coreboot[master]: soc/intel/tigerlake: Avoid NULL pointer dereference

[John Zhao (Code Review)]

John Zhao has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39260 )

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

soc/intel/tigerlake: Avoid NULL pointer dereference

Coverity detects pointer dev as FORWARD_NULL. Add sanity check
for dev to prevent NULL pointer dereference.

BUG=CID 1353148
TEST=Built and boot up to kernel.

Change-Id: Ic0ad1ec79c950a3c17feccdde4f87f4a107fe8c0
Signed-off-by: John Zhao 
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
1 file changed, 6 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39260/1

diff --git a/src/soc/intel/tigerlake/fsp_params_jsl.c b/src/soc/intel/tigerlake/fsp_params_jsl.c
index 8eb3fba..39ef66e 100644
--- a/src/soc/intel/tigerlake/fsp_params_jsl.c
+++ b/src/soc/intel/tigerlake/fsp_params_jsl.c
@@ -158,9 +158,12 @@

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
-	if (!dev || !xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (!dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+
+		params->XdciEnable = dev->enabled;
+	}

/* Provide correct UART number for FSP debug logs */
    params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE;

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39260 )

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params_jsl.c:

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 161: if (!dev) {
             : 		if (!xdci_can_enable())
             : 			dev->enabled = 0;
             : 
             : 		params->XdciEnable = dev->enabled;
             : 	}
This is still not correct. You are accessing dev if it is NULL.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39260 )

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

Patch Set 1: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params_jsl.c:

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 163: dev
If dev is NULL you are still accessing a NULL pointer here and on line 165.

[John Zhao (Code Review)]

Hello build bot (Jenkins), Martin Roth, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

soc/intel/tigerlake: Avoid NULL pointer dereference

Coverity detects pointer dev as FORWARD_NULL. Add sanity check
for dev to prevent NULL pointer dereference.

BUG=CID 1353148
TEST=Built and boot up to kernel.

Change-Id: Ic0ad1ec79c950a3c17feccdde4f87f4a107fe8c0
Signed-off-by: John Zhao 
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 11 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39260/2

[John Zhao (Code Review)]

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

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params_jsl.c:

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 163: dev
> If dev is NULL you are still accessing a NULL pointer here and on line 165.

Ack

https://review.coreboot.org/c/coreboot/+/39260/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 161: if (!dev) {
             : 		if (!xdci_can_enable())
             : 			dev->enabled = 0;
             : 
             : 		params->XdciEnable = dev->enabled;
             : 	}
> This is still not correct. You are accessing dev if it is NULL.

Ack

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39260 )

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39260/2/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params_jsl.c:

https://review.coreboot.org/c/coreboot/+/39260/2/src/soc/intel/tigerlake/fsp... 
PS2, Line 166: 	}
Don't you need an else block to set params->XdciEnable to 0 if dev is NULL?

[John Zhao (Code Review)]

Hello build bot (Jenkins), Martin Roth, Aaron Durbin, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

soc/intel/tigerlake: Avoid NULL pointer dereference

Coverity detects pointer dev as FORWARD_NULL. Add sanity check
for dev to prevent NULL pointer dereference.

BUG=CID 1353148
TEST=Built and boot up to kernel.

Change-Id: Ic0ad1ec79c950a3c17feccdde4f87f4a107fe8c0
Signed-off-by: John Zhao 
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 15 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/60/39260/3

[John Zhao (Code Review)]

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

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39260/2/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params_jsl.c:

https://review.coreboot.org/c/coreboot/+/39260/2/src/soc/intel/tigerlake/fsp... 
PS2, Line 166: 	}
> Don't you need an else block to set params->XdciEnable to 0 if dev is NULL?

Ack

[Angel Pons (Code Review)]

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

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

Patch Set 3: Code-Review+2

Oops

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/tigerlake: Avoid NULL pointer dereference
......................................................................

soc/intel/tigerlake: Avoid NULL pointer dereference

Coverity detects pointer dev as FORWARD_NULL. Add sanity check
for dev to prevent NULL pointer dereference.

BUG=CID 1353148
TEST=Built and boot up to kernel.

Change-Id: Ic0ad1ec79c950a3c17feccdde4f87f4a107fe8c0
Signed-off-by: John Zhao 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39260
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/soc/intel/tigerlake/fsp_params_jsl.c
M src/soc/intel/tigerlake/fsp_params_tgl.c
2 files changed, 15 insertions(+), 6 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/tigerlake/fsp_params_jsl.c b/src/soc/intel/tigerlake/fsp_params_jsl.c
index 8eb3fba..6cb3b67 100644
--- a/src/soc/intel/tigerlake/fsp_params_jsl.c
+++ b/src/soc/intel/tigerlake/fsp_params_jsl.c
@@ -158,9 +158,14 @@

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
-	if (!dev || !xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+
+		params->XdciEnable = dev->enabled;
+	} else {
+		params->XdciEnable = 0;
+	}

/* Provide correct UART number for FSP debug logs */
    params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE;
diff --git a/src/soc/intel/tigerlake/fsp_params_tgl.c b/src/soc/intel/tigerlake/fsp_params_tgl.c
index 0587b88..9e22b58 100644
--- a/src/soc/intel/tigerlake/fsp_params_tgl.c
+++ b/src/soc/intel/tigerlake/fsp_params_tgl.c
@@ -115,9 +115,13 @@

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_on_root(PCH_DEV_SLOT_XHCI, 1);
-	if (!xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+		params->XdciEnable = dev->enabled;
+	} else {
+		params->XdciEnable = 0;
+	}

/* PCH UART selection for FSP Debug */
    params->SerialIoDebugUartNumber = CONFIG_UART_FOR_CONSOLE;