On Mon, May 18, 2015 at 09:56:33PM +0300, Dmitry Kalinkin wrote:
> +static int vme_user_sg_to_dma_list(const struct vme_dma_op *dma_op,
> + struct sg_table *sgt, int sg_count, struct vme_dma_list *dma_list)
> +{
> + ssize_t pos = 0;
> + struct scatterlist *sg;
> + int i, ret;
> +
> + 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).
> + dma_op->aspace, dma_op->cycle, dma_op->dwidth);
> + if (!vme_attr)
> + return -ENOMEM;
> +
> + pci_attr = vme_dma_pci_attribute(hw_address);
> + if (!pci_attr) {
> + vme_dma_free_attribute(vme_attr);
> + return -ENOMEM;
> + }
> +
> + if (dma_op->write) {
> + dest = vme_attr;
> + src = pci_attr;
> + } else {
> + dest = pci_attr;
> + src = vme_attr;
> + }
> +
> + ret = vme_dma_list_add(dma_list, src, dest, hw_len);
> +
> + /*
> + * XXX VME API doesn't mention whether we should keep
> + * attributes around
> + */
> + vme_dma_free_attribute(vme_attr);
> + vme_dma_free_attribute(pci_attr);
> +
> + if (ret)
> + return ret;
> +
> + pos += hw_len;
> + }
> +
> + WARN_ON(pos != dma_op->count);
> +
> + return 0;
> +}
> +
> +static ssize_t vme_user_dma_ioctl(unsigned int minor,
> + const struct vme_dma_op *dma_op)
> +{
> + unsigned int offset = offset_in_page(dma_op->buf_vaddr);
> + unsigned long nr_pages;
> + enum dma_data_direction dir;
> + struct vme_dma_list *dma_list;
> + struct sg_table *sgt = NULL;
> + struct page **pages = NULL;
> + long got_pages;
> + int ret, sg_count;
> +
> + /* Overflow check for nr_pages */
> + if (dma_op->count > U32_MAX - 2 * PAGE_SIZE)
> + return -EINVAL;
> +
> + /* Prevent WARN from dma_map_sg */
> + if (dma_op->count == 0)
> + return 0;
> +
> + 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?
> + if (!pages) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> + if (!sgt) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + dma_list = vme_new_dma_list(image[minor].resource);
> + if (!dma_list) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> + got_pages = get_user_pages_fast(dma_op->buf_vaddr, nr_pages,
> + !dma_op->write, pages);
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);
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
