Change in coreboot[master]: soc/nvidia/tegra210: Fix potential NULL pointer dereference

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

soc/nvidia/tegra210: Fix potential NULL pointer dereference

Recent Coverity scan indicated potential NULL deference; spi->dma_in and
spi->dma_out are set to NULL after a cycle finishes, and a spurious call
to this could segfault

Change-Id: Icd1412f0956c0a4a75266d1873d5e9848aceee32
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/nvidia/tegra210/spi.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34787/1

diff --git a/src/soc/nvidia/tegra210/spi.c b/src/soc/nvidia/tegra210/spi.c
index 9310e0c..d392f70 100644
--- a/src/soc/nvidia/tegra210/spi.c
+++ b/src/soc/nvidia/tegra210/spi.c
@@ -574,6 +574,11 @@

struct apb_dma * const apb_dma = (struct apb_dma *)TEGRA_APB_DMA_BASE;

+	if (spi->dma_in == NULL || spi->dma_out == NULL) {
+		ret = -1;
+		goto done;
+	}
+
    todo = read32(&spi->dma_in->regs->wcount);

if (spi->dma_in) {

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34787/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34787/1//COMMIT_MSG@12 
PS1, Line 12: 
Please add a tag line: Found-by: Coverity …

See `git log` for some examples.

[Tim Wawrzynczak (Code Review)]

Hello HAOUAS Elyes, Julius Werner, Christian Walter, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

soc/nvidia/tegra210: Fix potential NULL pointer dereference

Recent Coverity scan indicated potential NULL deference; spi->dma_in and
spi->dma_out are set to NULL after a cycle finishes, and a spurious call
to this could segfault

Found-by: Coverity CID 1241838, 1241854
Change-Id: Icd1412f0956c0a4a75266d1873d5e9848aceee32
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/nvidia/tegra210/spi.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34787/2

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34787/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/34787/1//COMMIT_MSG@12 
PS1, Line 12:
> Please add a tag line: Found-by: Coverity … […]

Done

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 2: Code-Review+1

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34787/2/src/soc/nvidia/tegra210/spi... 
File src/soc/nvidia/tegra210/spi.c:

https://review.coreboot.org/c/coreboot/+/34787/2/src/soc/nvidia/tegra210/spi... 
PS2, Line 584: if (spi->dma_in) {
This if statement can be removed now

https://review.coreboot.org/c/coreboot/+/34787/2/src/soc/nvidia/tegra210/spi... 
PS2, Line 596: if (spi->dma_out) {
Same as this one

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34787/2/src/soc/nvidia/tegra210/spi... 
File src/soc/nvidia/tegra210/spi.c:

https://review.coreboot.org/c/coreboot/+/34787/2/src/soc/nvidia/tegra210/spi... 
PS2, Line 584: if (spi->dma_in) {
> This if statement can be removed now

Done

https://review.coreboot.org/c/coreboot/+/34787/2/src/soc/nvidia/tegra210/spi... 
PS2, Line 596: if (spi->dma_out) {
> Same as this one

Done

[Tim Wawrzynczak (Code Review)]

Hello HAOUAS Elyes, Julius Werner, Christian Walter, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

soc/nvidia/tegra210: Fix potential NULL pointer dereference

Recent Coverity scan indicated potential NULL deference; spi->dma_in and
spi->dma_out are set to NULL after a cycle finishes, and a spurious call
to this could segfault

Found-by: Coverity CID 1241838, 1241854
Change-Id: Icd1412f0956c0a4a75266d1873d5e9848aceee32
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/nvidia/tegra210/spi.c
1 file changed, 25 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34787/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34787/3/src/soc/nvidia/tegra210/spi... 
File src/soc/nvidia/tegra210/spi.c:

https://review.coreboot.org/c/coreboot/+/34787/3/src/soc/nvidia/tegra210/spi... 
PS3, Line 585: 	while ((read32(&spi->dma_in->regs->dma_byte_sta) < todo) ||
suspect code indent for conditional statements (8, 10)

https://review.coreboot.org/c/coreboot/+/34787/3/src/soc/nvidia/tegra210/spi... 
PS3, Line 596: 	while ((read32(&spi->dma_out->regs->dma_byte_sta) < todo) ||
suspect code indent for conditional statements (8, 10)

[Tim Wawrzynczak (Code Review)]

Hello HAOUAS Elyes, Julius Werner, Christian Walter, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

soc/nvidia/tegra210: Fix potential NULL pointer dereference

Recent Coverity scan indicated potential NULL deference; spi->dma_in and
spi->dma_out are set to NULL after a cycle finishes, and a spurious call
to this could segfault

Found-by: Coverity CID 1241838, 1241854
Change-Id: Icd1412f0956c0a4a75266d1873d5e9848aceee32
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/nvidia/tegra210/spi.c
1 file changed, 25 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34787/4

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/34787/3/src/soc/nvidia/tegra210/spi... 
File src/soc/nvidia/tegra210/spi.c:

https://review.coreboot.org/c/coreboot/+/34787/3/src/soc/nvidia/tegra210/spi... 
PS3, Line 585: 	while ((read32(&spi->dma_in->regs->dma_byte_sta) < todo) ||
> suspect code indent for conditional statements (8, 10)

Done

https://review.coreboot.org/c/coreboot/+/34787/3/src/soc/nvidia/tegra210/spi... 
PS3, Line 596: 	while ((read32(&spi->dma_out->regs->dma_byte_sta) < todo) ||
> suspect code indent for conditional statements (8, 10)

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 4: Code-Review-1

I don't think this is correct. Looks like depending on whether the transfer is in or out, only one of the two may be non-NULL at a given time. I think the correct fix would be to make dump_dma_regs() check for NULL.

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 4:
> Patch Set 4: Code-Review-1
>
> I don't think this is correct. Looks like depending on whether the transfer is in or out, only one of the two may be non-NULL at a given time. I think the correct fix would be to make dump_dma_regs() check for NULL.

I see now, sorry.  Good call.

[Tim Wawrzynczak (Code Review)]

Hello HAOUAS Elyes, Julius Werner, Christian Walter, build bot (Jenkins), Martin Roth, Patrick Georgi,

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

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

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

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

soc/nvidia/tegra210: Fix potential NULL pointer dereference

Recent Coverity scan indicated potential NULL deference; if either
spi->dma_in or spi->dma_out are NULL, the fifo_error() check could
dereference a NULL pointer.

Also fixed what appears to be a logic bug for the spi->dma_out case,
where it was using the todo (count) from spi->dma_in.

Found-by: Coverity CID 1241838, 1241854
Change-Id: Icd1412f0956c0a4a75266d1873d5e9848aceee32
Signed-off-by: Tim Wawrzynczak 
---
M src/soc/nvidia/tegra210/spi.c
1 file changed, 25 insertions(+), 21 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34787/5

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

Patch Set 6: Code-Review+2

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34787 )

Change subject: soc/nvidia/tegra210: Fix potential NULL pointer dereference
......................................................................

soc/nvidia/tegra210: Fix potential NULL pointer dereference

Recent Coverity scan indicated potential NULL deference; if either
spi->dma_in or spi->dma_out are NULL, the fifo_error() check could
dereference a NULL pointer.

Also fixed what appears to be a logic bug for the spi->dma_out case,
where it was using the todo (count) from spi->dma_in.

Found-by: Coverity CID 1241838, 1241854
Change-Id: Icd1412f0956c0a4a75266d1873d5e9848aceee32
Signed-off-by: Tim Wawrzynczak 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34787
Reviewed-by: Julius Werner 
Tested-by: build bot (Jenkins) 
---
M src/soc/nvidia/tegra210/spi.c
1 file changed, 25 insertions(+), 21 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved

diff --git a/src/soc/nvidia/tegra210/spi.c b/src/soc/nvidia/tegra210/spi.c
index 9310e0c..6ec8b64 100644
--- a/src/soc/nvidia/tegra210/spi.c
+++ b/src/soc/nvidia/tegra210/spi.c
@@ -286,25 +286,27 @@

static void dump_dma_regs(struct apb_dma_channel *dma)
 {
-	printk(BIOS_INFO, "DMA regs:\n"
-			"\tahb_ptr: 0x%08x\n"
-			"\tapb_ptr: 0x%08x\n"
-			"\tahb_seq: 0x%08x\n"
-			"\tapb_seq: 0x%08x\n"
-			"\tcsr: 0x%08x\n"
-			"\tcsre: 0x%08x\n"
-			"\twcount: 0x%08x\n"
-			"\tdma_byte_sta: 0x%08x\n"
-			"\tword_transfer: 0x%08x\n",
-			read32(&dma->regs->ahb_ptr),
-			read32(&dma->regs->apb_ptr),
-			read32(&dma->regs->ahb_seq),
-			read32(&dma->regs->apb_seq),
-			read32(&dma->regs->csr),
-			read32(&dma->regs->csre),
-			read32(&dma->regs->wcount),
-			read32(&dma->regs->dma_byte_sta),
-			read32(&dma->regs->word_transfer));
+	if (dma) {
+		printk(BIOS_INFO, "DMA regs:\n"
+		       "\tahb_ptr: 0x%08x\n"
+		       "\tapb_ptr: 0x%08x\n"
+		       "\tahb_seq: 0x%08x\n"
+		       "\tapb_seq: 0x%08x\n"
+		       "\tcsr: 0x%08x\n"
+		       "\tcsre: 0x%08x\n"
+		       "\twcount: 0x%08x\n"
+		       "\tdma_byte_sta: 0x%08x\n"
+		       "\tword_transfer: 0x%08x\n",
+		       read32(&dma->regs->ahb_ptr),
+		       read32(&dma->regs->apb_ptr),
+		       read32(&dma->regs->ahb_seq),
+		       read32(&dma->regs->apb_seq),
+		       read32(&dma->regs->csr),
+		       read32(&dma->regs->csre),
+		       read32(&dma->regs->wcount),
+		       read32(&dma->regs->dma_byte_sta),
+		       read32(&dma->regs->word_transfer));
+	}
 }

static inline unsigned int spi_byte_count(struct tegra_spi_channel *spi)
@@ -574,9 +576,9 @@

struct apb_dma * const apb_dma = (struct apb_dma *)TEGRA_APB_DMA_BASE;

-	todo = read32(&spi->dma_in->regs->wcount);
-
    if (spi->dma_in) {
+		todo = read32(&spi->dma_in->regs->wcount);
+
    	while ((read32(&spi->dma_in->regs->dma_byte_sta) < todo) ||
    			dma_busy(spi->dma_in))
    		;
@@ -589,6 +591,8 @@
    }

if (spi->dma_out) {
+		todo = read32(&spi->dma_out->regs->wcount);
+
    	while ((read32(&spi->dma_out->regs->dma_byte_sta) < todo) ||
    			dma_busy(spi->dma_out))
    		;