diff mbox series

[3/3] spi: spi-mem: Add extra sanity checks on the op param

Message ID 20180920073112.7897-4-boris.brezillon@bootlin.com (mailing list archive)
State Accepted
Commit 380583227c0c7f52383b0cd5c0e2de93ed31d553
Headers show
Series spi: spi-mem: Minor fixes/improvements | expand

Commit Message

Boris Brezillon Sept. 20, 2018, 7:31 a.m. UTC
Some combinations are simply not valid and should be rejected before
the op is passed to the SPI controller driver.

Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
spi_mem_supports_op() to make sure the spi-mem operation is valid.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/spi/spi-mem.c | 54 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

Comments

Yogesh Narayan Gaur Sept. 20, 2018, 10:31 a.m. UTC | #1
Hi Boris,

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
> Boris Brezillon
> Sent: Thursday, September 20, 2018 1:01 PM
> To: Mark Brown <broonie@kernel.org>; linux-spi@vger.kernel.org
> Cc: Marek Vasut <marek.vasut@gmail.com>; Richard Weinberger
> <richard@nod.at>; Boris Brezillon <boris.brezillon@bootlin.com>; linux-
> mtd@lists.infradead.org; Brian Norris <computersforpeace@gmail.com>; David
> Woodhouse <dwmw2@infradead.org>
> Subject: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param
> 
> Some combinations are simply not valid and should be rejected before the op is
> passed to the SPI controller driver.
> 
> Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
> spi_mem_supports_op() to make sure the spi-mem operation is valid.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/spi/spi-mem.c | 54
> +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> eb72dba71d83..cc3d425aae56 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -12,6 +12,8 @@
> 
>  #include "internals.h"
> 
> +#define SPI_MEM_MAX_BUSWIDTH		4
> +
>  /**
>   * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to
> a
>   *					  memory operation
> @@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct
> spi_mem *mem,  }  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> 
> +static bool spi_mem_buswidth_is_valid(u8 buswidth) {
> +	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)

Isn't check for lower limit should be '< 1'?

--
Regards
Yogesh Gaur

> +		return false;
> +
> +	return true;
> +}
> +
> +static int spi_mem_check_op(const struct spi_mem_op *op) {
> +	if (!op->cmd.buswidth)
> +		return -EINVAL;
> +
> +	if ((op->addr.nbytes && !op->addr.buswidth) ||
> +	    (op->dummy.nbytes && !op->dummy.buswidth) ||
> +	    (op->data.nbytes && !op->data.buswidth))
> +		return -EINVAL;
> +
> +	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->data.buswidth))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static bool spi_mem_internal_supports_op(struct spi_mem *mem,
> +					 const struct spi_mem_op *op)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +
> +	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
> +		return ctlr->mem_ops->supports_op(mem, op);
> +
> +	return spi_mem_default_supports_op(mem, op); }
> +
>  /**
>   * spi_mem_supports_op() - Check if a memory device and the controller it is
>   *			   connected to support a specific memory operation
> @@ -166,12 +206,10 @@
> EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>   */
>  bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op
> *op)  {
> -	struct spi_controller *ctlr = mem->spi->controller;
> -
> -	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
> -		return ctlr->mem_ops->supports_op(mem, op);
> +	if (spi_mem_check_op(op))
> +		return false;
> 
> -	return spi_mem_default_supports_op(mem, op);
> +	return spi_mem_internal_supports_op(mem, op);
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> 
> @@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  	u8 *tmpbuf;
>  	int ret;
> 
> -	if (!spi_mem_supports_op(mem, op))
> +	ret = spi_mem_check_op(op);
> +	if (ret)
> +		return ret;
> +
> +	if (!spi_mem_internal_supports_op(mem, op))
>  		return -ENOTSUPP;
> 
>  	if (ctlr->mem_ops) {
> --
> 2.14.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fmailman%2Flistinfo%2Flinux-
> mtd%2F&amp;data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C3593fc7
> 7e9d44b5b7a6e08d61ecb6c1c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C636730256384380635&amp;sdata=lTp45z8K1WFhI0LA7Nxca2p8pdHsQL
> gXNFC1GJIi1NM%3D&amp;reserved=0
Boris Brezillon Sept. 20, 2018, 11:14 a.m. UTC | #2
Hi Yogesh,

On Thu, 20 Sep 2018 10:31:16 +0000
Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com> wrote:

> > 
> > +static bool spi_mem_buswidth_is_valid(u8 buswidth) {
> > +	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)  
> 
> Isn't check for lower limit should be '< 1'?

buswidth == 0 is valid if you have nothing to send...

> 
> --
> Regards
> Yogesh Gaur
> 
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static int spi_mem_check_op(const struct spi_mem_op *op) {
> > +	if (!op->cmd.buswidth)
> > +		return -EINVAL;
> > +
> > +	if ((op->addr.nbytes && !op->addr.buswidth) ||
> > +	    (op->dummy.nbytes && !op->dummy.buswidth) ||
> > +	    (op->data.nbytes && !op->data.buswidth))
> > +		return -EINVAL;

... and I make sure op->data.buswidth != 0 when required here.

Thanks for the review by the way.

Boris
Geert Uytterhoeven Sept. 25, 2018, 9:40 a.m. UTC | #3
Hi Boris,

On Thu, Sep 20, 2018 at 9:32 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Some combinations are simply not valid and should be rejected before
> the op is passed to the SPI controller driver.
>
> Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
> spi_mem_supports_op() to make sure the spi-mem operation is valid.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

This is now commit 380583227c0c7f52 ("spi: spi-mem: Add extra sanity checks
on the op param") in spi/for-next, and causes probing of the QSPI FLASH
on r8a7791/koelsch to fail with:

    m25p80 spi0.0: error -22 reading 9f
    m25p80: probe of spi0.0 failed with error -22

Reverting the commit revives the FLASH:

    m25p80 spi0.0: s25fl512s (65536 Kbytes)
    3 fixed-partitions partitions found on MTD device spi0.0
    Creating 3 MTD partitions on "spi0.0":
    0x000000000000-0x000000080000 : "loader"
    0x000000080000-0x000000600000 : "user"
    0x000000600000-0x000004000000 : "flash"

> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -12,6 +12,8 @@
>
>  #include "internals.h"
>
> +#define SPI_MEM_MAX_BUSWIDTH           4
> +
>  /**
>   * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
>   *                                       memory operation
> @@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>
> +static bool spi_mem_buswidth_is_valid(u8 buswidth)
> +{
> +       if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
> +               return false;
> +
> +       return true;
> +}
> +
> +static int spi_mem_check_op(const struct spi_mem_op *op)
> +{
> +       if (!op->cmd.buswidth)
> +               return -EINVAL;
> +
> +       if ((op->addr.nbytes && !op->addr.buswidth) ||
> +           (op->dummy.nbytes && !op->dummy.buswidth) ||
> +           (op->data.nbytes && !op->data.buswidth))
> +               return -EINVAL;
> +
> +       if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->addr.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->data.buswidth))

Adding debug code shows:

    op->cmd.buswidth = 1
    op->addr.buswidth = 0
    op->dummy.buswidth = 0
    op->data.buswidth = 1

so they're all valid, leading to a failure?!?
Looks like the logic is completely reversed, will send a patch...

> +               return -EINVAL;
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index eb72dba71d83..cc3d425aae56 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -12,6 +12,8 @@ 
 
 #include "internals.h"
 
+#define SPI_MEM_MAX_BUSWIDTH		4
+
 /**
  * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
  *					  memory operation
@@ -149,6 +151,44 @@  static bool spi_mem_default_supports_op(struct spi_mem *mem,
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
 
+static bool spi_mem_buswidth_is_valid(u8 buswidth)
+{
+	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
+		return false;
+
+	return true;
+}
+
+static int spi_mem_check_op(const struct spi_mem_op *op)
+{
+	if (!op->cmd.buswidth)
+		return -EINVAL;
+
+	if ((op->addr.nbytes && !op->addr.buswidth) ||
+	    (op->dummy.nbytes && !op->dummy.buswidth) ||
+	    (op->data.nbytes && !op->data.buswidth))
+		return -EINVAL;
+
+	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->data.buswidth))
+		return -EINVAL;
+
+	return 0;
+}
+
+static bool spi_mem_internal_supports_op(struct spi_mem *mem,
+					 const struct spi_mem_op *op)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
+		return ctlr->mem_ops->supports_op(mem, op);
+
+	return spi_mem_default_supports_op(mem, op);
+}
+
 /**
  * spi_mem_supports_op() - Check if a memory device and the controller it is
  *			   connected to support a specific memory operation
@@ -166,12 +206,10 @@  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
  */
 bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
-	struct spi_controller *ctlr = mem->spi->controller;
-
-	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
-		return ctlr->mem_ops->supports_op(mem, op);
+	if (spi_mem_check_op(op))
+		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	return spi_mem_internal_supports_op(mem, op);
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
@@ -196,7 +234,11 @@  int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	u8 *tmpbuf;
 	int ret;
 
-	if (!spi_mem_supports_op(mem, op))
+	ret = spi_mem_check_op(op);
+	if (ret)
+		return ret;
+
+	if (!spi_mem_internal_supports_op(mem, op))
 		return -ENOTSUPP;
 
 	if (ctlr->mem_ops) {