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 <[email protected]>
---
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;
+
return 0;
}
--
2.34.1
Hi Pratyush,
[email protected] 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 <[email protected]>
> ---
> 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
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 <[email protected]>
> + /* 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?
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/[email protected]/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 <[email protected]>
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/[email protected]
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 <[email protected]>
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.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
On 01/02/22 07:44AM, [email protected] 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 <[email protected]>
> > ---
> > 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...
--
Regards,
Pratyush Yadav
Texas Instruments Inc.