[PATCH] mmc: warn for invalid SDIO data buffers

[PATCH] mmc: warn for invalid SDIO data buffers

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Jernej Skrabec reported a problem with the cw1200 driver failing on
arm64 systems with CONFIG_VMAP_STACK=y.

The driver in this case passes a pointer to a stack variable (in vmalloc
space) into the sdio layer, which gets translated into an invalid DMA
address.

Even without CONFIG_VMAP_STACK, the driver is still unreliable, as
cache invalidations on the DMA buffer may cause random data corruption
in adjacent stack slots.

This could be worked around in the SDIO core, but in the discussion we
decided that passing a stack variable into SDIO should always be considered
a bug, as it is for USB drivers.

Change the sdio core to produce a one-time warning for any on-stack
(both with and without CONFIG_VMAP_STACK) as well as any vmalloc
or module-local address that would have the same translation problem.

Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Link: https://lore.kernel.org/lkml/20210622202345.795578-1-jernej.skrabec@gmail.com/
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mmc/core/sdio_ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 4c229dd2b6e5..14e983faf223 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/scatterlist.h>
+#include <linux/sched/task_stack.h>
 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
@@ -124,6 +125,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
 	int err;
 
 	WARN_ON(blksz == 0);
+	WARN_ON_ONCE(is_vmalloc_or_module_addr(buf) || object_is_on_stack(buf));
 
 	/* sanity check */
 	if (addr & ~0x1FFFF)
-- 
2.29.2

Re: [PATCH] mmc: warn for invalid SDIO data buffers

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1465 bytes --]

Hi Arnd,

I love your patch! Yet something to improve:

[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v5.13 next-20210630]
[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/Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237
base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: i386-randconfig-c001-20210630 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/256b826ee105fe46723b99c3f128ea01aa3e7adf
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237
        git checkout 256b826ee105fe46723b99c3f128ea01aa3e7adf
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

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 >>, old ones prefixed by <<):

>> ERROR: modpost: "is_vmalloc_or_module_addr" [drivers/mmc/core/mmc_core.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 44843 bytes --]

Re: [PATCH] mmc: warn for invalid SDIO data buffers

[Arnd Bergmann]

On Thu, Jul 1, 2021 at 1:02 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Arnd,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on soc/for-next]
> [also build test ERROR on linus/master v5.13 next-20210630]
> [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/Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
> config: i386-randconfig-c001-20210630 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build):
>         # https://github.com/0day-ci/linux/commit/256b826ee105fe46723b99c3f128ea01aa3e7adf
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Arnd-Bergmann/mmc-warn-for-invalid-SDIO-data-buffers/20210630-202237
>         git checkout 256b826ee105fe46723b99c3f128ea01aa3e7adf
>         # save the attached .config to linux build tree
>         make W=1 ARCH=i386
>
> 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 >>, old ones prefixed by <<):
>
> >> ERROR: modpost: "is_vmalloc_or_module_addr" [drivers/mmc/core/mmc_core.ko] undefined!

Ah, I see that is_vmalloc_addr() is exported, but is_vmalloc_or_module_addr()
is not. I assume it's ok to add a corresponding EXPORT_SYMBOL_GPL() for
it as well. I'll wait for other comments before resending with that change.

      Arnd

Re: [PATCH] mmc: warn for invalid SDIO data buffers【请注意，邮件由linux-mmc-owner@vger.kernel.org代发】

[Shawn Lin]

Hi Arnd

On 2021/6/30 20:20, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Jernej Skrabec reported a problem with the cw1200 driver failing on
> arm64 systems with CONFIG_VMAP_STACK=y.
> 
> The driver in this case passes a pointer to a stack variable (in vmalloc
> space) into the sdio layer, which gets translated into an invalid DMA
> address.
> 
> Even without CONFIG_VMAP_STACK, the driver is still unreliable, as
> cache invalidations on the DMA buffer may cause random data corruption
> in adjacent stack slots.
> 
> This could be worked around in the SDIO core, but in the discussion we
> decided that passing a stack variable into SDIO should always be considered
> a bug, as it is for USB drivers.
> 
> Change the sdio core to produce a one-time warning for any on-stack
> (both with and without CONFIG_VMAP_STACK) as well as any vmalloc
> or module-local address that would have the same translation problem.

This was the previous comment about the same topic.
Should we check for mmc_io_rw_direct?

https://www.spinics.net/lists/linux-mmc/msg41794.html

> 
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Link: https://lore.kernel.org/lkml/20210622202345.795578-1-jernej.skrabec@gmail.com/
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/mmc/core/sdio_ops.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
> index 4c229dd2b6e5..14e983faf223 100644
> --- a/drivers/mmc/core/sdio_ops.c
> +++ b/drivers/mmc/core/sdio_ops.c
> @@ -6,6 +6,7 @@
>    */
>   
>   #include <linux/scatterlist.h>
> +#include <linux/sched/task_stack.h>
>   
>   #include <linux/mmc/host.h>
>   #include <linux/mmc/card.h>
> @@ -124,6 +125,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,
>   	int err;
>   
>   	WARN_ON(blksz == 0);
> +	WARN_ON_ONCE(is_vmalloc_or_module_addr(buf) || object_is_on_stack(buf));
>   
>   	/* sanity check */
>   	if (addr & ~0x1FFFF)
> 

Re: [PATCH] mmc: warn for invalid SDIO data buffers【请注意，邮件由linux-mmc-owner@vger.kernel.org代发】

[Arnd Bergmann]

On Fri, Jul 2, 2021 at 3:02 AM Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2021/6/30 20:20, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > Jernej Skrabec reported a problem with the cw1200 driver failing on
> > arm64 systems with CONFIG_VMAP_STACK=y.
> >
> > The driver in this case passes a pointer to a stack variable (in vmalloc
> > space) into the sdio layer, which gets translated into an invalid DMA
> > address.
> >
> > Even without CONFIG_VMAP_STACK, the driver is still unreliable, as
> > cache invalidations on the DMA buffer may cause random data corruption
> > in adjacent stack slots.
> >
> > This could be worked around in the SDIO core, but in the discussion we
> > decided that passing a stack variable into SDIO should always be considered
> > a bug, as it is for USB drivers.
> >
> > Change the sdio core to produce a one-time warning for any on-stack
> > (both with and without CONFIG_VMAP_STACK) as well as any vmalloc
> > or module-local address that would have the same translation problem.
>
> This was the previous comment about the same topic.
> Should we check for mmc_io_rw_direct?
>
> https://www.spinics.net/lists/linux-mmc/msg41794.html

Hi Shawn,

thank you for remembering that previous discussion, that is a
good question. Looking at the code though, I don't actually
see any part of mmc_io_rw_direct() doing DMA on a caller-provided
buffer. The only thing I see in the code is a 'u8 *out' argument, but
that is just a pointer to a single byte that is set by this function.

Do you see any other issue with that function, or does that mean
we don't have to change it?

       Arnd

Re: [PATCH] mmc: warn for invalid SDIO data buffers【请注意，邮件由linux-mmc-owner@vger.kernel.org代发】

[Shawn Lin]

On 2021/7/2 15:03, Arnd Bergmann wrote:
> On Fri, Jul 2, 2021 at 3:02 AM Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> On 2021/6/30 20:20, Arnd Bergmann wrote:
>>> From: Arnd Bergmann <arnd@arndb.de>
>>>
>>> Jernej Skrabec reported a problem with the cw1200 driver failing on
>>> arm64 systems with CONFIG_VMAP_STACK=y.
>>>
>>> The driver in this case passes a pointer to a stack variable (in vmalloc
>>> space) into the sdio layer, which gets translated into an invalid DMA
>>> address.
>>>
>>> Even without CONFIG_VMAP_STACK, the driver is still unreliable, as
>>> cache invalidations on the DMA buffer may cause random data corruption
>>> in adjacent stack slots.
>>>
>>> This could be worked around in the SDIO core, but in the discussion we
>>> decided that passing a stack variable into SDIO should always be considered
>>> a bug, as it is for USB drivers.
>>>
>>> Change the sdio core to produce a one-time warning for any on-stack
>>> (both with and without CONFIG_VMAP_STACK) as well as any vmalloc
>>> or module-local address that would have the same translation problem.
>>
>> This was the previous comment about the same topic.
>> Should we check for mmc_io_rw_direct?
>>
>> https://www.spinics.net/lists/linux-mmc/msg41794.html
> 
> Hi Shawn,
> 
> thank you for remembering that previous discussion, that is a
> good question. Looking at the code though, I don't actually
> see any part of mmc_io_rw_direct() doing DMA on a caller-provided
> buffer. The only thing I see in the code is a 'u8 *out' argument, but
> that is just a pointer to a single byte that is set by this function.
> 

I didn't quite get the point we need check out for
mmc_io_rw_direct() either. As I thought what Ulf was suggesting was some
controllers might use data fifo to get cmd response and get DMA
involved? But I don't see a explicit one.

> Do you see any other issue with that function, or does that mean
> we don't have to change it?
> 
>         Arnd
> 
> 
> 

Re: [PATCH] mmc: warn for invalid SDIO data buffers【请注意，邮件由linux-mmc-owner@vger.kernel.org代发】

[Ulf Hansson]

On Fri, 2 Jul 2021 at 09:03, Arnd Bergmann <arnd@kernel.org> wrote:
>
> On Fri, Jul 2, 2021 at 3:02 AM Shawn Lin <shawn.lin@rock-chips.com> wrote:
> > On 2021/6/30 20:20, Arnd Bergmann wrote:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Jernej Skrabec reported a problem with the cw1200 driver failing on
> > > arm64 systems with CONFIG_VMAP_STACK=y.
> > >
> > > The driver in this case passes a pointer to a stack variable (in vmalloc
> > > space) into the sdio layer, which gets translated into an invalid DMA
> > > address.
> > >
> > > Even without CONFIG_VMAP_STACK, the driver is still unreliable, as
> > > cache invalidations on the DMA buffer may cause random data corruption
> > > in adjacent stack slots.
> > >
> > > This could be worked around in the SDIO core, but in the discussion we
> > > decided that passing a stack variable into SDIO should always be considered
> > > a bug, as it is for USB drivers.
> > >
> > > Change the sdio core to produce a one-time warning for any on-stack
> > > (both with and without CONFIG_VMAP_STACK) as well as any vmalloc
> > > or module-local address that would have the same translation problem.
> >
> > This was the previous comment about the same topic.
> > Should we check for mmc_io_rw_direct?
> >
> > https://www.spinics.net/lists/linux-mmc/msg41794.html
>
> Hi Shawn,
>
> thank you for remembering that previous discussion, that is a
> good question. Looking at the code though, I don't actually
> see any part of mmc_io_rw_direct() doing DMA on a caller-provided
> buffer. The only thing I see in the code is a 'u8 *out' argument, but
> that is just a pointer to a single byte that is set by this function.
>
> Do you see any other issue with that function, or does that mean
> we don't have to change it?

I was wrong when I earlier said that we needed to care about
mmc_io_rw_direct(). mmc_io_rw_direct() transfer "data" over the CMD
line. MMC host drivers can't do DMA on that.

I think the $subject patch looks reasonable to me.

Kind regards
Uffe