Re: [PATCH 6/6] staging: vme_user: provide DMA functionality

[Dmitry Kalinkin <dmitry.kalinkin@gmail.com>]

On Tue, May 19, 2015 at 12:18 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, May 18, 2015 at 09:56:33PM +0300, Dmitry Kalinkin wrote:
>> 
>> +     for_each_sg(sgt->sgl, sg, sg_count, i) {
>> +             struct vme_dma_attr *pci_attr, *vme_attr, *dest, *src;
>> +             dma_addr_t hw_address = sg_dma_address(sg);
>> +             unsigned int hw_len = sg_dma_len(sg);
>> +
>> +             vme_attr = vme_dma_vme_attribute(dma_op->vme_addr + pos,
>                                                 ^^^^^^^^^^^^^^^^^^^^^^
> 
> ->vme_addr comes from the user and we don't seem to have done any
> validation that it's correct.  This addition can overflow.  How is this
> safe?  (This is not a rhetorical question, I am a newbie in this).
> 
This expression calculates address on the VME bus, where we already have
full access. There shouldn't have security implications. Such transfer will
most likely wrap or cause DMA transfer error (gives EIO).

We could add an additional check:

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index d8aa03b..cb0fc63 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -515,6 +515,11 @@ static ssize_t vme_user_dma_ioctl(unsigned int minor,
        if (dma_op->count == 0)
                return 0;
 
+       ret = vme_check_window(dma_op->aspace, dma_op->vme_addr,
+                              dma_op->count);
+       if (ret)
+               return ret;
+
        nr_pages = (offset + dma_op->count + PAGE_SIZE - 1) >> PAGE_SHIFT;
        dir = dma_op->write ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
 
diff --git a/drivers/vme/vme.c b/drivers/vme/vme.c
index 6bab2c4..50cabc3 100644
--- a/drivers/vme/vme.c
+++ b/drivers/vme/vme.c
@@ -177,7 +177,7 @@ size_t vme_get_size(struct vme_resource *resource)
 }
 EXPORT_SYMBOL(vme_get_size);
 
-static int vme_check_window(u32 aspace, unsigned long long vme_base,
+int vme_check_window(u32 aspace, unsigned long long vme_base,
        unsigned long long size)
 {
        int retval = 0;
@@ -199,10 +199,8 @@ static int vme_check_window(u32 aspace, unsigned long long vme_base,
                        retval = -EFAULT;
                break;
        case VME_A64:
-               /*
-                * Any value held in an unsigned long long can be used as the
-                * base
-                */
+               if (vme_base > VME_A64_MAX - size)
+                       retval = -EFAULT;
                break;
        case VME_CRCSR:
                if (((vme_base + size) > VME_CRCSR_MAX) ||
@@ -223,6 +221,7 @@ static int vme_check_window(u32 aspace, unsigned long long vme_base,
 
        return retval;
 }
+EXPORT_SYMBOL(vme_check_window);
 
 /*
  * Request a slave image with specific attributes, return some unique
diff --git a/include/linux/vme.h b/include/linux/vme.h
index 79242e9..cfff272 100644
--- a/include/linux/vme.h
+++ b/include/linux/vme.h
@@ -120,6 +120,8 @@ void vme_free_consistent(struct vme_resource *, size_t,  void *,
        dma_addr_t);
 
 size_t vme_get_size(struct vme_resource *);
+int vme_check_window(u32 aspace, unsigned long long vme_base,
+                     unsigned long long size);
 
 struct vme_resource *vme_slave_request(struct vme_dev *, u32, u32);
 int vme_slave_set(struct vme_resource *, int, unsigned long long,

>> +     nr_pages = (offset + dma_op->count + PAGE_SIZE - 1) >> PAGE_SHIFT;
>> +     dir = dma_op->write ? DMA_TO_DEVICE : DMA_FROM_DEVICE;
>> +
>> +     pages = kmalloc_array(nr_pages, sizeof(pages[0]), GFP_KERNEL);
> 
> This lets the user try allocate huge ammounts of RAM.  Is there no
> reasonable max size we can use?
> 
We could limit operation size by 64 MiB and do an partial read for any bigger request.

> This file is all indented poorly, but these patches adds a bunch of new
> ones so they make a bad situation worse.
> 
>        got_pages = get_user_pages_fast(dma_op->buf_vaddr, nr_pages,
>                                        !dma_op->write, pages);
> 
> You sometimes might have to use spaces to make things align correctly.
> 
>        got_pages = some_fake_name(dma_op->buf_vaddr, nr_pages,
>                                   !dma_op->write, pages);
> 
> [tab][tab][tab][tab][space][space][space][space]!dma_op->write, pages);
Will fix this.


Thank you.

Cheers,
Dmitry