linux-kernel - Re: [PATCH v7 00/36] DRM: fix struct sg_table nents vs. orig

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]

Message-ID: <10cb27c9-286a-5ee5-dab1-183939e5b12d@samsung.com>
Date:   Tue, 30 Jun 2020 10:49:42 +0200
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     dri-devel@...ts.freedesktop.org, iommu@...ts.linux-foundation.org,
        linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org
Cc:     Christoph Hellwig <hch@....de>,
        Robin Murphy <robin.murphy@....com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        linux-arm-kernel@...ts.infradead.org,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v7 00/36] DRM: fix struct sg_table nents vs. orig_nents
 misuse

Hi All,

On 19.06.2020 12:36, Marek Szyprowski wrote:
> During the Exynos DRM GEM rework and fixing the issues in the.
> drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most
> drivers in DRM framework incorrectly use nents and orig_nents entries of
> the struct sg_table.
>
> In case of the most DMA-mapping implementations exchanging those two
> entries or using nents for all loops on the scatterlist is harmless,
> because they both have the same value. There exists however a DMA-mapping
> implementations, for which such incorrect usage breaks things. The nents
> returned by dma_map_sg() might be lower than the nents passed as its
> parameter and this is perfectly fine. DMA framework or IOMMU is allowed
> to join consecutive chunks while mapping if such operation is supported
> by the underlying HW (bus, bridge, IOMMU, etc). Example of the case
> where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages
> is described here [2]
>
> The DMA-mapping framework documentation [3] states that dma_map_sg()
> returns the numer of the created entries in the DMA address space.
> However the subsequent calls to dma_sync_sg_for_{device,cpu} and
> dma_unmap_sg must be called with the original number of entries passed to
> dma_map_sg. The common pattern in DRM drivers were to assign the
> dma_map_sg() return value to sg_table->nents and use that value for
> the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also
> the code iterated over nents times to access the pages stored in the
> processed scatterlist, while it should use orig_nents as the numer of
> the page entries.
>
> I've tried to identify all such incorrect usage of sg_table->nents and
> this is a result of my research. It looks that the incorrect pattern has
> been copied over the many drivers mainly in the DRM subsystem. Too bad in
> most cases it even worked correctly if the system used a simple, linear
> DMA-mapping implementation, for which swapping nents and orig_nents
> doesn't make any difference. To avoid similar issues in the future, I've
> introduced a common wrappers for DMA-mapping calls, which operate directly
> on the sg_table objects. I've also added wrappers for iterating over the
> scatterlists stored in the sg_table objects and applied them where
> possible. This, together with some common DRM prime helpers, allowed me
> to almost get rid of all nents/orig_nents usage in the drivers. I hope
> that such change makes the code robust, easier to follow and copy/paste
> safe.
>
> The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix
> it fully. The driver creatively uses sg_table->orig_nents to store the
> size of the allocate scatterlist and ignores the number of the entries
> returned by dma_map_sg function. In this patchset I only fixed the
> sg_table objects exported by dmabuf related functions. I hope that I
> didn't break anything there.
>
> Patches are based on top of Linux next-20200618. The required changes to
> DMA-mapping framework has been already merged to v5.8-rc1.
>
> If possible I would like ask for merging most of the patches via DRM
> tree.

David & Daniel: how would you like to merge those patches? They got 
quite a lot acks and some of them have dependencies on the DRM core. I 
would really like to get patches 1-28 merged via DRM (misc?) tree. Do 
you want me to prepare a branch and send a pull request?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland