Re: [PATCH v4] mmc: sdio: check the buffer address for sdio API

[Russell King - ARM Linux <linux@armlinux.org.uk>]

On Tue, Feb 14, 2017 at 09:18:43AM -0700, Jens Axboe wrote:
> The current situation seems like a bit of a mess. Why don't you have two
> entry points, one for DMA and one for PIO. If the caller doesn't know if
> he can use DMA, he'd better call the PIO variant. Either that, or audit
> all callers and ensure they do the right thing wrt having a dma capable
> buffer.

It really shouldn't matter.  MMC interfaces are just like USB - you
have a host controller, which interfaces what is a multi-lane serial
bus to the system.  The SDIO card shouldn't care one bit whether
the host controller is using DMA or PIO.

However, I think that the DMA vs PIO thing is actually misleading here,
that's really not the issue at all.

Looking at the oops, I see it uses sdio_memcpy_toio().  Tracing that
code leads me to here:

                for_each_sg(data.sg, sg_ptr, data.sg_len, i) {
                        sg_set_page(sg_ptr, virt_to_page(buf + (i * seg_size)),
                                        min(seg_size, left_size),
                                        offset_in_page(buf + (i * seg_size)));

so the buffer that is passed into sdio_memcpy_toio() gets passed into
virt_to_page().

Firstly, the fact that it's passed to virt_to_page() means that "buf"
must _only_ _ever_ be a lowmem address.  It can't ever be a vmalloc
address (virt_to_page() is invalid on anything but lowmem.)  Just like
certain kernel interfaces, passing pointers to memory of different types
from the one intended by the interface produces invalid results, and
that seems to be what's happening here.

Secondly, it's a scatterlist, and scatterlists can be passed to DMA
mapping operations, which also implies that _if_ a host driver decides
to use DMA on it, the buffer better be DMA-able.

Thirdly, while PIO may work (or even appear to work) because _maybe_
converting a vmalloc address to a ficticious struct page + offset, and
then converting that back again _might_ result in hitting the correct
memory, but it's not guaranteed to.

I suspect that virt_to_page() + kmap_atomic() is likely to try to
dereference a struct page pointer that does not point at a legal entry
in the memmap arrays, and result in scribbling over some random part
of kernel memory.

So every way I look at this, the binary driver that Shawn downloaded
is buggy, whether the host controller uses PIO or DMA.

I bet if Shawn tries running it against a modern kernel with
CONFIG_DEBUG_VIRTUAL enabled, Shawn will get complaints backing up
my claim.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.