Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35904 )
Change subject: soc/nvidia/tegra124: Fix null pointer dereference ......................................................................
soc/nvidia/tegra124: Fix null pointer dereference
Commit 680027edf6 (soc/nvidia/tegra210: Fix potential NULL pointer dereference) fixed a null dereference and logic bug in the tegra210 spi code. Coverity is warning about the same problem for tegra124, so apply the same fix there.
Change-Id: I6a7403417ee83b703cf4ca495129f73c66691ea9 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 124183, 124185 --- M src/soc/nvidia/tegra124/spi.c 1 file changed, 25 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35904/1
diff --git a/src/soc/nvidia/tegra124/spi.c b/src/soc/nvidia/tegra124/spi.c index 2d6469c..da9f4cd 100644 --- a/src/soc/nvidia/tegra124/spi.c +++ b/src/soc/nvidia/tegra124/spi.c @@ -288,25 +288,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) @@ -545,9 +547,9 @@ int ret; unsigned int todo;
- 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)) ; /* this shouldn't take long, no udelay */ @@ -557,6 +559,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)) spi_delay(spi, todo - spi_byte_count(spi));
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35904 )
Change subject: soc/nvidia/tegra124: Fix null pointer dereference ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... File src/soc/nvidia/tegra124/spi.c:
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 291: if (dma) { I might have done
if (!dma) { printk(); return; }
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 564: while ((read32(&spi->dma_out->regs->dma_byte_sta) < todo) || The value 'todo' changes here, so this commit is no longer just a NULL check. My first guess is your change is correct though..
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 566: spi_delay(spi, todo - spi_byte_count(spi)); I would like to see braces with this 'while'.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35904 )
Change subject: soc/nvidia/tegra124: Fix null pointer dereference ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35904/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35904/1//COMMIT_MSG@13 PS1, Line 13: Please duplicate the other commit message here.
Hello Julius Werner, Tim Wawrzynczak, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35904
to look at the new patch set (#2).
Change subject: soc/nvidia/tegra124: Fix null pointer and logic bug ......................................................................
soc/nvidia/tegra124: Fix null pointer and logic bug
Commit 680027edf6 fixed a null dereference and logic bug in the tegra210 spi code:
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.
Coverity is warning about the same problem for tegra124, so apply the same fix there. Also, add braces around a while statement.
Change-Id: I6a7403417ee83b703cf4ca495129f73c66691ea9 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 124183, 124185 --- M src/soc/nvidia/tegra124/spi.c 1 file changed, 9 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/04/35904/2
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35904 )
Change subject: soc/nvidia/tegra124: Fix null pointer and logic bug ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35904/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35904/1//COMMIT_MSG@13 PS1, Line 13:
Please duplicate the other commit message here.
Done
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... File src/soc/nvidia/tegra124/spi.c:
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 291: if (dma) {
I might have done […]
Done
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 564: while ((read32(&spi->dma_out->regs->dma_byte_sta) < todo) ||
The value 'todo' changes here, so this commit is no longer just a NULL check. […]
This is the fix for the logic bug, which was copied from the previous commit.
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 566: spi_delay(spi, todo - spi_byte_count(spi));
I would like to see braces with this 'while'.
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35904 )
Change subject: soc/nvidia/tegra124: Fix null pointer and logic bug ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... File src/soc/nvidia/tegra124/spi.c:
https://review.coreboot.org/c/coreboot/+/35904/1/src/soc/nvidia/tegra124/spi... PS1, Line 564: while ((read32(&spi->dma_out->regs->dma_byte_sta) < todo) ||
This is the fix for the logic bug, which was copied from the previous commit.
The call to spi_delay() below makes this a bit different logic bug. But I guess this is the best we can do without someone actually triggering the bug first to document the symptoms.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35904 )
Change subject: soc/nvidia/tegra124: Fix null pointer and logic bug ......................................................................
soc/nvidia/tegra124: Fix null pointer and logic bug
Commit 680027edf6 fixed a null dereference and logic bug in the tegra210 spi code:
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.
Coverity is warning about the same problem for tegra124, so apply the same fix there. Also, add braces around a while statement.
Change-Id: I6a7403417ee83b703cf4ca495129f73c66691ea9 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Found-by: Coverity CID 124183, 124185 Reviewed-on: https://review.coreboot.org/c/coreboot/+/35904 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/nvidia/tegra124/spi.c 1 file changed, 9 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, approved
diff --git a/src/soc/nvidia/tegra124/spi.c b/src/soc/nvidia/tegra124/spi.c index 2d6469c..27ae1fa 100644 --- a/src/soc/nvidia/tegra124/spi.c +++ b/src/soc/nvidia/tegra124/spi.c @@ -288,6 +288,9 @@
static void dump_dma_regs(struct apb_dma_channel *dma) { + if (dma == NULL) + return; + printk(BIOS_INFO, "DMA regs:\n" "\tahb_ptr: 0x%08x\n" "\tapb_ptr: 0x%08x\n" @@ -545,9 +548,9 @@ int ret; unsigned int todo;
- 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)) ; /* this shouldn't take long, no udelay */ @@ -557,9 +560,12 @@ }
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)) + dma_busy(spi->dma_out)) { spi_delay(spi, todo - spi_byte_count(spi)); + } clrbits_le32(&spi->regs->command1, SPI_CMD1_TX_EN); dma_stop(spi->dma_out); dma_release(spi->dma_out);
-
Jacob Garber (Code Review) -
Kyösti Mälkki (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)