diff mbox series

spi: spi-mem: check if data buffers are on stack

Message ID 20220131114508.1028306-1-p.yadav@ti.com (mailing list archive)
State Superseded
Headers show
Series spi: spi-mem: check if data buffers are on stack | expand

Commit Message

Pratyush Yadav Jan. 31, 2022, 11:45 a.m. UTC 
The buffers passed in the data phase must be DMA-able. Programmers often
don't realise this requirement and pass in buffers that reside on the
stack. This can be hard to spot when reviewing code. Reject ops if their
data buffer is on the stack to avoid this.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/spi/spi-mem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Miquel Raynal Jan. 31, 2022, 1:48 p.m. UTC | #1 
Hi Pratyush,

p.yadav@ti.com wrote on Mon, 31 Jan 2022 17:15:08 +0530:

> The buffers passed in the data phase must be DMA-able. Programmers often
> don't realise this requirement and pass in buffers that reside on the
> stack. This can be hard to spot when reviewing code. Reject ops if their
> data buffer is on the stack to avoid this.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/spi/spi-mem.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 37f4443ce9a0..b3793a2979ee 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -207,6 +207,15 @@ static int spi_mem_check_op(const struct spi_mem_op *op)
>  	    !spi_mem_buswidth_is_valid(op->data.buswidth))
>  		return -EINVAL;
>  
> +	/* Buffers must be DMA-able. */
> +	if (op->data.dir == SPI_MEM_DATA_IN &&
> +	    object_is_on_stack(op->data.buf.in))
> +		return -EINVAL;
> +
> +	if (op->data.dir == SPI_MEM_DATA_OUT &&
> +	    object_is_on_stack(op->data.buf.out))
> +		return -EINVAL;

Definitely a good idea.

This change will depend on the spi-mem-ecc series. I will soon merge
this branch into mtd/next so that any change that depends on it can be
merged in mtd/next directly, if nobody disagrees.

Thanks,
Miquèl
Mark Brown Jan. 31, 2022, 1:55 p.m. UTC | #2 
On Mon, Jan 31, 2022 at 05:15:08PM +0530, Pratyush Yadav wrote:
> The buffers passed in the data phase must be DMA-able. Programmers often
> don't realise this requirement and pass in buffers that reside on the
> stack. This can be hard to spot when reviewing code. Reject ops if their
> data buffer is on the stack to avoid this.

Acked-by: Mark Brown <broonie@kernel.org>

> +	/* Buffers must be DMA-able. */
> +	if (op->data.dir == SPI_MEM_DATA_IN &&
> +	    object_is_on_stack(op->data.buf.in))

Might be worth a WARN_ON_ONCE() for debuggability?
kernel test robot Jan. 31, 2022, 2:49 p.m. UTC | #3 
Hi Pratyush,

I love your patch! Yet something to improve:

[auto build test ERROR on broonie-spi/for-next]
[also build test ERROR on v5.17-rc2 next-20220131]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Pratyush-Yadav/spi-spi-mem-check-if-data-buffers-are-on-stack/20220131-195211
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: arm-socfpga_defconfig (https://download.01.org/0day-ci/archive/20220131/202201312233.QPZMOWRk-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 2cdbaca3943a4d6259119f185656328bd3805b68)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/0b93f667f8445e744ca4b8f80ce9a1ad4c981a2e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pratyush-Yadav/spi-spi-mem-check-if-data-buffers-are-on-stack/20220131-195211
        git checkout 0b93f667f8445e744ca4b8f80ce9a1ad4c981a2e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/spi/spi-mem.c:212:6: error: implicit declaration of function 'object_is_on_stack' [-Werror,-Wimplicit-function-declaration]
               object_is_on_stack(op->data.buf.in))
               ^
   1 error generated.


vim +/object_is_on_stack +212 drivers/spi/spi-mem.c

   193	
   194	static int spi_mem_check_op(const struct spi_mem_op *op)
   195	{
   196		if (!op->cmd.buswidth || !op->cmd.nbytes)
   197			return -EINVAL;
   198	
   199		if ((op->addr.nbytes && !op->addr.buswidth) ||
   200		    (op->dummy.nbytes && !op->dummy.buswidth) ||
   201		    (op->data.nbytes && !op->data.buswidth))
   202			return -EINVAL;
   203	
   204		if (!spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
   205		    !spi_mem_buswidth_is_valid(op->addr.buswidth) ||
   206		    !spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
   207		    !spi_mem_buswidth_is_valid(op->data.buswidth))
   208			return -EINVAL;
   209	
   210		/* Buffers must be DMA-able. */
   211		if (op->data.dir == SPI_MEM_DATA_IN &&
 > 212		    object_is_on_stack(op->data.buf.in))
   213			return -EINVAL;
   214	
   215		if (op->data.dir == SPI_MEM_DATA_OUT &&
   216		    object_is_on_stack(op->data.buf.out))
   217			return -EINVAL;
   218	
   219		return 0;
   220	}
   221	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Pratyush Yadav Jan. 31, 2022, 5:07 p.m. UTC | #4 
On 31/01/22 01:55PM, Mark Brown wrote:
> On Mon, Jan 31, 2022 at 05:15:08PM +0530, Pratyush Yadav wrote:
> > The buffers passed in the data phase must be DMA-able. Programmers often
> > don't realise this requirement and pass in buffers that reside on the
> > stack. This can be hard to spot when reviewing code. Reject ops if their
> > data buffer is on the stack to avoid this.
> 
> Acked-by: Mark Brown <broonie@kernel.org>

Thanks. But seems like this is breaking build on arm-socfpga_defconfig. 
Let me take a look into it.

> 
> > +	/* Buffers must be DMA-able. */
> > +	if (op->data.dir == SPI_MEM_DATA_IN &&
> > +	    object_is_on_stack(op->data.buf.in))
> 
> Might be worth a WARN_ON_ONCE() for debuggability?

Okay, I'll add it.
Tudor Ambarus Feb. 1, 2022, 7:44 a.m. UTC | #5 
On 1/31/22 13:45, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The buffers passed in the data phase must be DMA-able. Programmers often
> don't realise this requirement and pass in buffers that reside on the
> stack. This can be hard to spot when reviewing code. Reject ops if their
> data buffer is on the stack to avoid this.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/spi/spi-mem.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 37f4443ce9a0..b3793a2979ee 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -207,6 +207,15 @@ static int spi_mem_check_op(const struct spi_mem_op *op)
>             !spi_mem_buswidth_is_valid(op->data.buswidth))
>                 return -EINVAL;
> 
> +       /* Buffers must be DMA-able. */
> +       if (op->data.dir == SPI_MEM_DATA_IN &&
> +           object_is_on_stack(op->data.buf.in))

should we also check if the virt addr is valid?
if (object_is_on_stack(op->data.buf.in) || !virt_addr_valid(op->data.buf.in))
Pratyush Yadav Feb. 1, 2022, 8:02 a.m. UTC | #6 
On 01/02/22 07:44AM, Tudor.Ambarus@microchip.com wrote:
> On 1/31/22 13:45, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > The buffers passed in the data phase must be DMA-able. Programmers often
> > don't realise this requirement and pass in buffers that reside on the
> > stack. This can be hard to spot when reviewing code. Reject ops if their
> > data buffer is on the stack to avoid this.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/spi/spi-mem.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index 37f4443ce9a0..b3793a2979ee 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -207,6 +207,15 @@ static int spi_mem_check_op(const struct spi_mem_op *op)
> >             !spi_mem_buswidth_is_valid(op->data.buswidth))
> >                 return -EINVAL;
> > 
> > +       /* Buffers must be DMA-able. */
> > +       if (op->data.dir == SPI_MEM_DATA_IN &&
> > +           object_is_on_stack(op->data.buf.in))
> 
> should we also check if the virt addr is valid?
> if (object_is_on_stack(op->data.buf.in) || !virt_addr_valid(op->data.buf.in))

When would virt addr not be valid? When someone passes a bad pointer? I 
generally have not seen kernel APIs checking for pointer validity (other 
than NULL). If you pass a bad pointer then expect bad things to happen. 
Plus a bad pointer might also point to a valid virtual address, and we 
have no way of catching that. Dunno...
diff mbox series

Patch

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 37f4443ce9a0..b3793a2979ee 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -207,6 +207,15 @@  static int spi_mem_check_op(const struct spi_mem_op *op)
 	    !spi_mem_buswidth_is_valid(op->data.buswidth))
 		return -EINVAL;
 
+	/* Buffers must be DMA-able. */
+	if (op->data.dir == SPI_MEM_DATA_IN &&
+	    object_is_on_stack(op->data.buf.in))
+		return -EINVAL;
+
+	if (op->data.dir == SPI_MEM_DATA_OUT &&
+	    object_is_on_stack(op->data.buf.out))
+		return -EINVAL;
+
 	return 0;
 }