Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756008AbbEUWMR (ORCPT ); Thu, 21 May 2015 18:12:17 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:33750 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755163AbbEUWMO convert rfc822-to-8bit (ORCPT ); Thu, 21 May 2015 18:12:14 -0400 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\)) Subject: Re: [PATCH 6/6] staging: vme_user: provide DMA functionality From: Dmitry Kalinkin In-Reply-To: <20150519091846.GB4150@mwanda> Date: Fri, 22 May 2015 01:12:19 +0300 Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, Martyn Welch , Greg Kroah-Hartman , Manohar Vanga , Igor Alekseev Content-Transfer-Encoding: 8BIT Message-Id: References: <1431975393-19735-1-git-send-email-dmitry.kalinkin@gmail.com> <1431975393-19735-7-git-send-email-dmitry.kalinkin@gmail.com> <20150519091846.GB4150@mwanda> To: Dan Carpenter X-Mailer: Apple Mail (2.2098) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4603 Lines: 120 On Tue, May 19, 2015 at 12:18 PM, Dan Carpenter 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/