linux-kernel - Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 21 Mar 2019 15:35:10 -0500
From:   "Andrew F. Davis" <afd@...com>
To:     Brian Starkey <Brian.Starkey@....com>,
        John Stultz <john.stultz@...aro.org>
CC:     lkml <linux-kernel@...r.kernel.org>,
        Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Chenbo Feng <fengc@...gle.com>,
        Alistair Strachan <astrachan@...gle.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        nd <nd@....com>
Subject: Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers

On 3/19/19 9:26 AM, Brian Starkey wrote:
> Hi John,
> 
> On Tue, Mar 05, 2019 at 12:54:30PM -0800, John Stultz wrote:
> 
> ...
> 
>> +
>> +void dma_heap_buffer_destroy(struct dma_heap_buffer *heap_buffer)
>> +{
>> +	struct heap_helper_buffer *buffer = to_helper_buffer(heap_buffer);
>> +
>> +	if (buffer->kmap_cnt > 0) {
>> +		pr_warn_once("%s: buffer still mapped in the kernel\n",
>> +			     __func__);
> 
> Could be worth something louder like a full WARN.
> 
>> +		vunmap(buffer->vaddr);
>> +	}
>> +
>> +	buffer->free(buffer);
>> +}
>> +
> 
> ...
> 
>> +
>> +static void *dma_heap_dma_buf_kmap(struct dma_buf *dmabuf,
>> +					unsigned long offset)
>> +{
>> +	struct dma_heap_buffer *heap_buffer = dmabuf->priv;
>> +	struct heap_helper_buffer *buffer = to_helper_buffer(heap_buffer);
>> +
>> +	return buffer->vaddr + offset * PAGE_SIZE;
> 
> I think it'd be good to check for NULL vaddr and return NULL in that
> case. Less chance of an invalid pointer being accidentally used then.
> 

Why do we assume vaddr is set at all here? I'm guessing we expected
dma_heap_map_kernel to have been called, that is not going to always be
the case. kmap should perform it's own single page kmap here and not
rely on the clunky full buffer vmap (which is probably broken on 32bit
systems here when the buffers are large).

Andrew

> Thanks,
> -Brian
>