Hi! > When handling op->addr, it is using the buffer "tmpbuf" which has been > freed. This will trigger a use-after-free KASAN warning. Let's use > temporary variables to store op->addr.val and op->cmd.opcode to fix > this issue. I believe this is "cure worse than a disassease". > +++ b/drivers/spi/spi-zynqmp-gqspi.c > @@ -926,8 +926,9 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem, > struct zynqmp_qspi *xqspi = spi_controller_get_devdata > (mem->spi->master); > int err = 0, i; > - u8 *tmpbuf; > u32 genfifoentry = 0; > + u16 opcode = op->cmd.opcode; > + u64 opaddr; > > dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n", > op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth, > @@ -940,14 +941,8 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem, > genfifoentry |= xqspi->genfifobus; > > if (op->cmd.opcode) { > - tmpbuf = kzalloc(op->cmd.nbytes, GFP_KERNEL | GFP_DMA); > - if (!tmpbuf) { > - mutex_unlock(&xqspi->op_lock); > - return -ENOMEM; > - } > - tmpbuf[0] = op->cmd.opcode; > reinit_completion(&xqspi->data_completion); > - xqspi->txbuf = tmpbuf; > + xqspi->txbuf = &opcode; > xqspi->rxbuf = NULL; > xqspi->bytes_to_transfer = op->cmd.nbytes; > xqspi->bytes_to_receive = 0; So this replaces "op->cmd.nbytes" bytes big DMA buffer with 2 bytes on stack. First, if op->cmd.nbytes is > 2, DMA will overrun that buffer. That can't be healthy. Second, you really should not run DMA from on-stack buffers. Best regards, Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c index d6ac8fe145a1..2a0be16b2eb0 100644 --- a/drivers/spi/spi-zynqmp-gqspi.c +++ b/drivers/spi/spi-zynqmp-gqspi.c @@ -926,8 +926,9 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem, struct zynqmp_qspi *xqspi = spi_controller_get_devdata (mem->spi->master); int err = 0, i; - u8 *tmpbuf; u32 genfifoentry = 0; + u16 opcode = op->cmd.opcode; + u64 opaddr; dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n", op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth, @@ -940,14 +941,8 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem, genfifoentry |= xqspi->genfifobus; if (op->cmd.opcode) { - tmpbuf = kzalloc(op->cmd.nbytes, GFP_KERNEL | GFP_DMA); - if (!tmpbuf) { - mutex_unlock(&xqspi->op_lock); - return -ENOMEM; - } - tmpbuf[0] = op->cmd.opcode; reinit_completion(&xqspi->data_completion); - xqspi->txbuf = tmpbuf; + xqspi->txbuf = &opcode; xqspi->rxbuf = NULL; xqspi->bytes_to_transfer = op->cmd.nbytes; xqspi->bytes_to_receive = 0; @@ -961,13 +956,12 @@ static int zynqmp_qspi_exec_op(struct spi_mem *mem, if (!wait_for_completion_timeout (&xqspi->data_completion, msecs_to_jiffies(1000))) { err = -ETIMEDOUT; - kfree(tmpbuf); goto return_err; } - kfree(tmpbuf); } if (op->addr.nbytes) { + xqspi->txbuf = &opaddr; for (i = 0; i < op->addr.nbytes; i++) { *(((u8 *)xqspi->txbuf) + i) = op->addr.val >> (8 * (op->addr.nbytes - i - 1));