linux-kernel - Re: [RESEND][PATCH v8 4/5] dma-buf: heaps: Add CMA heap to dmabuf heaps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190923221013.ovwqdr34777cluj2@DESKTOP-E1NTVVP.localdomain>
Date:   Mon, 23 Sep 2019 22:10:19 +0000
From:   Brian Starkey <Brian.Starkey@....com>
To:     John Stultz <john.stultz@...aro.org>
CC:     lkml <linux-kernel@...r.kernel.org>,
        Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Pratik Patel <pratikp@...eaurora.org>,
        Vincent Donnefort <Vincent.Donnefort@....com>,
        Sudipto Paul <Sudipto.Paul@....com>,
        "Andrew F . Davis" <afd@...com>,
        Christoph Hellwig <hch@...radead.org>,
        Chenbo Feng <fengc@...gle.com>,
        Alistair Strachan <astrachan@...gle.com>,
        Hridya Valsaraju <hridya@...gle.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        nd <nd@....com>
Subject: Re: [RESEND][PATCH v8 4/5] dma-buf: heaps: Add CMA heap to dmabuf
 heaps

Hi John,

I spotted one thing below which might be harmless, but best to check.

On Fri, Sep 06, 2019 at 06:47:11PM +0000, John Stultz wrote:
> This adds a CMA heap, which allows userspace to allocate
> a dma-buf of contiguous memory out of a CMA region.
> 
> This code is an evolution of the Android ION implementation, so
> thanks to its original author and maintainters:
>   Benjamin Gaignard, Laura Abbott, and others!
> 
> Cc: Laura Abbott <labbott@...hat.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@...aro.org>
> Cc: Sumit Semwal <sumit.semwal@...aro.org>
> Cc: Liam Mark <lmark@...eaurora.org>
> Cc: Pratik Patel <pratikp@...eaurora.org>
> Cc: Brian Starkey <Brian.Starkey@....com>
> Cc: Vincent Donnefort <Vincent.Donnefort@....com>
> Cc: Sudipto Paul <Sudipto.Paul@....com>
> Cc: Andrew F. Davis <afd@...com>
> Cc: Christoph Hellwig <hch@...radead.org>
> Cc: Chenbo Feng <fengc@...gle.com>
> Cc: Alistair Strachan <astrachan@...gle.com>
> Cc: Hridya Valsaraju <hridya@...gle.com>
> Cc: dri-devel@...ts.freedesktop.org
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@...aro.org>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
> v2:
> * Switch allocate to return dmabuf fd
> * Simplify init code
> * Checkpatch fixups
> v3:
> * Switch to inline function for to_cma_heap()
> * Minor cleanups suggested by Brian
> * Fold in new registration style from Andrew
> * Folded in changes from Andrew to use simplified page list
>   from the heap helpers
> v4:
> * Use the fd_flags when creating dmabuf fd (Suggested by
>   Benjamin)
> * Use precalculated pagecount (Suggested by Andrew)
> v6:
> * Changed variable names to improve clarity, as suggested
>   by Brian
> v7:
> * Use newly lower-cased init_heap_helper_buffer helper
> * Use new dmabuf export helper
> v8:
> * Make struct dma_heap_ops const (Suggested by Christoph)
> * Condense dma_heap_buffer and heap_helper_buffer (suggested by
>   Christoph)
> * Checkpatch whitespace fixups
> ---

...

> +
> +/* dmabuf heap CMA operations functions */
> +static int cma_heap_allocate(struct dma_heap *heap,
> +			     unsigned long len,
> +			     unsigned long fd_flags,
> +			     unsigned long heap_flags)
> +{
> +	struct cma_heap *cma_heap = dma_heap_get_data(heap);
> +	struct heap_helper_buffer *helper_buffer;
> +	struct page *cma_pages;
> +	size_t size = PAGE_ALIGN(len);
> +	unsigned long nr_pages = size >> PAGE_SHIFT;
> +	unsigned long align = get_order(size);
> +	struct dma_buf *dmabuf;
> +	int ret = -ENOMEM;
> +	pgoff_t pg;
> +
> +	if (align > CONFIG_CMA_ALIGNMENT)
> +		align = CONFIG_CMA_ALIGNMENT;
> +
> +	helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> +	if (!helper_buffer)
> +		return -ENOMEM;
> +
> +	init_heap_helper_buffer(helper_buffer, cma_heap_free);
> +	helper_buffer->flags = heap_flags;
> +	helper_buffer->heap = heap;
> +	helper_buffer->size = len;
> +
> +	cma_pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> +	if (!cma_pages)
> +		goto free_buf;
> +
> +	if (PageHighMem(cma_pages)) {
> +		unsigned long nr_clear_pages = nr_pages;
> +		struct page *page = cma_pages;
> +
> +		while (nr_clear_pages > 0) {
> +			void *vaddr = kmap_atomic(page);
> +
> +			memset(vaddr, 0, PAGE_SIZE);
> +			kunmap_atomic(vaddr);
> +			page++;
> +			nr_clear_pages--;
> +		}
> +	} else {
> +		memset(page_address(cma_pages), 0, size);
> +	}
> +
> +	helper_buffer->pagecount = nr_pages;
> +	helper_buffer->pages = kmalloc_array(helper_buffer->pagecount,
> +					     sizeof(*helper_buffer->pages),
> +					     GFP_KERNEL);
> +	if (!helper_buffer->pages) {
> +		ret = -ENOMEM;
> +		goto free_cma;
> +	}
> +
> +	for (pg = 0; pg < helper_buffer->pagecount; pg++) {
> +		helper_buffer->pages[pg] = &cma_pages[pg];
> +		if (!helper_buffer->pages[pg])

Is this ever really possible? If cma_pages is non-NULL (which you
check earlier), then only if the pointer arithmetic overflows right?

If it's just redundant, then you could remove it (and in that case add
my r-b). But maybe you meant to check something else?

Cheers,
-Brian