linux-kernel - Re: [PATCH v3] dma-buf: cma_heap: Check for device max segment size when attaching

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] [day] [month] [year] [list]

Date:   Fri, 10 Mar 2023 10:54:57 -0600
From:   Andrew Davis <afd@...com>
To:     John Stultz <jstultz@...gle.com>
CC:     Sumit Semwal <sumit.semwal@...aro.org>,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        Liam Mark <lmark@...eaurora.org>,
        Brian Starkey <Brian.Starkey@....com>,
        Christian König <christian.koenig@....com>,
        <dri-devel@...ts.freedesktop.org>,
        <linaro-mm-sig@...ts.linaro.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] dma-buf: cma_heap: Check for device max segment size
 when attaching

On 3/6/23 8:48 PM, John Stultz wrote:
> On Mon, Mar 6, 2023 at 8:52 AM Andrew Davis <afd@...com> wrote:
>>
>> Although there is usually not such a limitation (and when there is it is
>> often only because the driver forgot to change the super small default),
>> it is still correct here to break scatterlist element into chunks of
>> dma_max_mapping_size().
> 
> Hey Andrew!
>    Thanks for sending this out!
> 
> So *why* is it "correct here to break scatterlist element into chunks
> of  dma_max_mapping_size()." ?
> 

Good question, I'm not 100% sure on the background myself. It seems
since some devices have restrictions on how large a mapping they can
handle in a single run, we should not hand out single scatterlist
elements longer than that.

It is still a contiguous buffer, but some drivers forget to set their
mapping limits and also only check the number of elements == 1 to determine
if a sg is contiguous (which is not correct as there is no rule that
contiguous runs must be merged into a single scatterlist). For those
driver this would be an issue (I've only found one such case in-tree and
sent a fix, https://lore.kernel.org/lkml/20220825162609.14076-1-afd@ti.com/)

>> This might cause some issues for users with misbehaving drivers. If
>> bisecting has landed you on this commit, make sure your drivers both set
>> dma_set_max_seg_size() and are checking for contiguousness correctly.
> 
> Why is this change worth the risk? (If this is really likely to break
> folks, should we maybe provide warnings initially instead? Maybe
> falling back to the old code if we can catch the failure?)
> 
> I don't really object to the change, just want to make sure the commit
> message is more clear on why we should make this change, what the
> benefit will be along with the potential downsides.
> 

I'm not sure it is worth the risk today either, but figured this being a
young enough exporter it could be a good spot to start with for exposing
misbehaving drivers vs some legacy GPU driver exporter. Plus better to
make this change now rather than later in any case.

I don't have any strong reason for this yet though, so I'm find with
just considering this patch an RFC for now.

Thanks,
Andrew

> thanks
> -john