* [PATCH] spi: spi-mem: check if data buffers are on stack
@ 2022-01-31 11:45 Pratyush Yadav
2022-01-31 13:48 ` Miquel Raynal
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Pratyush Yadav @ 2022-01-31 11:45 UTC (permalink / raw)
To: Mark Brown
Cc: Pratyush Yadav, Tudor Ambarus, Michael Walle, Miquel Raynal,
Takahiro Kuwano, linux-spi, linux-kernel
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;
+
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: spi-mem: check if data buffers are on stack
2022-01-31 11:45 [PATCH] spi: spi-mem: check if data buffers are on stack Pratyush Yadav
@ 2022-01-31 13:48 ` Miquel Raynal
2022-01-31 13:55 ` Mark Brown
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Miquel Raynal @ 2022-01-31 13:48 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Mark Brown, Tudor Ambarus, Michael Walle, Takahiro Kuwano,
linux-spi, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: spi-mem: check if data buffers are on stack
2022-01-31 11:45 [PATCH] spi: spi-mem: check if data buffers are on stack Pratyush Yadav
2022-01-31 13:48 ` Miquel Raynal
@ 2022-01-31 13:55 ` Mark Brown
2022-01-31 17:07 ` Pratyush Yadav
2022-01-31 14:49 ` kernel test robot
2022-02-01 7:44 ` Tudor.Ambarus
3 siblings, 1 reply; 7+ messages in thread
From: Mark Brown @ 2022-01-31 13:55 UTC (permalink / raw)
To: Pratyush Yadav
Cc: Tudor Ambarus, Michael Walle, Miquel Raynal, Takahiro Kuwano,
linux-spi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 551 bytes --]
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: spi-mem: check if data buffers are on stack
2022-01-31 11:45 [PATCH] spi: spi-mem: check if data buffers are on stack Pratyush Yadav
2022-01-31 13:48 ` Miquel Raynal
2022-01-31 13:55 ` Mark Brown
@ 2022-01-31 14:49 ` kernel test robot
2022-02-01 7:44 ` Tudor.Ambarus
3 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-01-31 14:49 UTC (permalink / raw)
To: Pratyush Yadav, Mark Brown
Cc: llvm, kbuild-all, Pratyush Yadav, Tudor Ambarus, Michael Walle,
Miquel Raynal, Takahiro Kuwano, linux-spi, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: spi-mem: check if data buffers are on stack
2022-01-31 13:55 ` Mark Brown
@ 2022-01-31 17:07 ` Pratyush Yadav
0 siblings, 0 replies; 7+ messages in thread
From: Pratyush Yadav @ 2022-01-31 17:07 UTC (permalink / raw)
To: Mark Brown
Cc: Tudor Ambarus, Michael Walle, Miquel Raynal, Takahiro Kuwano,
linux-spi, linux-kernel
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.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: spi-mem: check if data buffers are on stack
2022-01-31 11:45 [PATCH] spi: spi-mem: check if data buffers are on stack Pratyush Yadav
` (2 preceding siblings ...)
2022-01-31 14:49 ` kernel test robot
@ 2022-02-01 7:44 ` Tudor.Ambarus
2022-02-01 8:02 ` Pratyush Yadav
3 siblings, 1 reply; 7+ messages in thread
From: Tudor.Ambarus @ 2022-02-01 7:44 UTC (permalink / raw)
To: p.yadav, broonie
Cc: michael, miquel.raynal, tkuw584924, linux-spi, linux-kernel
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))
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] spi: spi-mem: check if data buffers are on stack
2022-02-01 7:44 ` Tudor.Ambarus
@ 2022-02-01 8:02 ` Pratyush Yadav
0 siblings, 0 replies; 7+ messages in thread
From: Pratyush Yadav @ 2022-02-01 8:02 UTC (permalink / raw)
To: Tudor.Ambarus
Cc: broonie, michael, miquel.raynal, tkuw584924, linux-spi, linux-kernel
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...
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-01 8:02 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-31 11:45 [PATCH] spi: spi-mem: check if data buffers are on stack Pratyush Yadav
2022-01-31 13:48 ` Miquel Raynal
2022-01-31 13:55 ` Mark Brown
2022-01-31 17:07 ` Pratyush Yadav
2022-01-31 14:49 ` kernel test robot
2022-02-01 7:44 ` Tudor.Ambarus
2022-02-01 8:02 ` Pratyush Yadav
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).