From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755635AbbESJUP (ORCPT ); Tue, 19 May 2015 05:20:15 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:28653 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932244AbbESJTW (ORCPT ); Tue, 19 May 2015 05:19:22 -0400 Date: Tue, 19 May 2015 12:18:47 +0300 From: Dan Carpenter To: Dmitry Kalinkin Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, Martyn Welch , Greg Kroah-Hartman , Manohar Vanga , Igor Alekseev Subject: Re: [PATCH 6/6] staging: vme_user: provide DMA functionality Message-ID: <20150519091846.GB4150@mwanda> References: <1431975393-19735-1-git-send-email-dmitry.kalinkin@gmail.com> <1431975393-19735-7-git-send-email-dmitry.kalinkin@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1431975393-19735-7-git-send-email-dmitry.kalinkin@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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