| Message ID | 20210914032148.273490-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | c2e0363571b124c4c543ad98d039d1eb319f1562 |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
| Series | mtd: cqspi: Fix division by zero | expand |
On 14/09/21 05:21AM, Marek Vasut wrote: > Both dummy.nbytes and dummy.buswidth may be zero. By not checking > the later, it is possible to trigger division by zero and a crash. > This does happen with tiny SPI NOR framework in SPL. Fix this by > adding the check and returning zero dummy bytes in such a case. > > Fixes: 38b0852b0ea ("spi: cadence-qspi: Add support for octal DTR flashes") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Vignesh R <vigneshr@ti.com> > Cc: Pratyush Yadav <p.yadav@ti.com> > --- > drivers/spi/cadence_qspi_apb.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > index c36a652211a..429ee335db6 100644 > --- a/drivers/spi/cadence_qspi_apb.c > +++ b/drivers/spi/cadence_qspi_apb.c > @@ -219,6 +219,9 @@ static unsigned int cadence_qspi_calc_dummy(const struct spi_mem_op *op, > { > unsigned int dummy_clk; > > + if (!op->dummy.nbytes || !op->dummy.buswidth) > + return 0; Thanks. A similar fix was sent for Linux as well [0]. I don't think you should check for dummy buswidth here since nbytes == 0 should be enough of an indicator that the dummy phase does not exist. Any op with dummy.nbytes > 1 and dummy.buswidth == 0 is a malformed op that should ideally never happen, or if it does, then it should be dealt by the SPI MEM core. With this fixed, Reviewed-by: Pratyush Yadav <p.yadav@ti.com> [0] https://patchwork.kernel.org/project/spi-devel-general/patch/92eea403-9b21-2488-9cc1-664bee760c5e@nskint.co.jp/ > + > dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); > if (dtr) > dummy_clk /= 2; > -- > 2.33.0 >
On 9/14/21 7:46 PM, Pratyush Yadav wrote: > On 14/09/21 05:21AM, Marek Vasut wrote: >> Both dummy.nbytes and dummy.buswidth may be zero. By not checking >> the later, it is possible to trigger division by zero and a crash. >> This does happen with tiny SPI NOR framework in SPL. Fix this by >> adding the check and returning zero dummy bytes in such a case. >> >> Fixes: 38b0852b0ea ("spi: cadence-qspi: Add support for octal DTR flashes") >> Signed-off-by: Marek Vasut <marex@denx.de> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Vignesh R <vigneshr@ti.com> >> Cc: Pratyush Yadav <p.yadav@ti.com> >> --- >> drivers/spi/cadence_qspi_apb.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c >> index c36a652211a..429ee335db6 100644 >> --- a/drivers/spi/cadence_qspi_apb.c >> +++ b/drivers/spi/cadence_qspi_apb.c >> @@ -219,6 +219,9 @@ static unsigned int cadence_qspi_calc_dummy(const struct spi_mem_op *op, >> { >> unsigned int dummy_clk; >> >> + if (!op->dummy.nbytes || !op->dummy.buswidth) >> + return 0; > > Thanks. A similar fix was sent for Linux as well [0]. I don't think you > should check for dummy buswidth here since nbytes == 0 should be enough > of an indicator that the dummy phase does not exist. Any op with > dummy.nbytes > 1 and dummy.buswidth == 0 is a malformed op that should > ideally never happen, or if it does, then it should be dealt by the SPI > MEM core. > > With this fixed, > > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> > > [0] https://patchwork.kernel.org/project/spi-devel-general/patch/92eea403-9b21-2488-9cc1-664bee760c5e@nskint.co.jp/ > >> + >> dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); Yes, indeed, the division by zero happens if op->dummy.buswidth == 0, so we must check it. Checking for op->dummy.nbytes == 0 is a minor optimization.
On Tue, Sep 14, 2021 at 05:21:48AM +0200, Marek Vasut wrote: > Both dummy.nbytes and dummy.buswidth may be zero. By not checking > the later, it is possible to trigger division by zero and a crash. > This does happen with tiny SPI NOR framework in SPL. Fix this by > adding the check and returning zero dummy bytes in such a case. > > Fixes: 38b0852b0ea ("spi: cadence-qspi: Add support for octal DTR flashes") > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Vignesh R <vigneshr@ti.com> > Cc: Pratyush Yadav <p.yadav@ti.com> Applied to u-boot/master, thanks!
diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index c36a652211a..429ee335db6 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -219,6 +219,9 @@ static unsigned int cadence_qspi_calc_dummy(const struct spi_mem_op *op, { unsigned int dummy_clk; + if (!op->dummy.nbytes || !op->dummy.buswidth) + return 0; + dummy_clk = op->dummy.nbytes * (8 / op->dummy.buswidth); if (dtr) dummy_clk /= 2;
Both dummy.nbytes and dummy.buswidth may be zero. By not checking the later, it is possible to trigger division by zero and a crash. This does happen with tiny SPI NOR framework in SPL. Fix this by adding the check and returning zero dummy bytes in such a case. Fixes: 38b0852b0ea ("spi: cadence-qspi: Add support for octal DTR flashes") Signed-off-by: Marek Vasut <marex@denx.de> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Vignesh R <vigneshr@ti.com> Cc: Pratyush Yadav <p.yadav@ti.com> --- drivers/spi/cadence_qspi_apb.c | 3 +++ 1 file changed, 3 insertions(+)