Date:   Mon, 15 Nov 2021 10:23:08 +0100
From:   Matthijs van Duin <matthijsvanduin@gmail.com>
To:     Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc:     Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>, airlied@linux.ie,
        daniel@ffwll.ch, linux-kernel@vger.kernel.org,
        linux-omap@vger.kernel.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v2] drm: omapdrm: Export correct scatterlist for TILER
 backed BOs
Message-ID: <YZIm/GiRWv0nPN5b@chinchilla>
Mail-Followup-To: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>, airlied@linux.ie,
        daniel@ffwll.ch, linux-kernel@vger.kernel.org,
        linux-omap@vger.kernel.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
References: <1636796417-5997-1-git-send-email-ivo.g.dimitrov.75@gmail.com>
 <1636797239-6384-1-git-send-email-ivo.g.dimitrov.75@gmail.com>
 <36598203-eced-131d-85ef-f4940872e751@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <36598203-eced-131d-85ef-f4940872e751@ideasonboard.com>
Precedence: bulk

On Mon, Nov 15, 2021 at 10:42:41AM +0200, Tomi Valkeinen wrote:
> A BO's memory via the TILER memory is
> contiguous, although with consistent gaps of
> memory that should not be accessed.

But pretending that these "gaps" are part of the buffer is a security
vulnerability, since that memory which "should not be accessed" may
belong to different security contexts, and exporting the entire
contiguous region covering the buffer allows untrusted contexts (e.g.
userspace) to access this memory.

> IPs that might use TILER
> backed BOs only support contiguous memory.
> 
> This means that the drivers for such IPs cannot
> use the BOs exported like you do in this patch.
> I believe the drivers could be improved by
> writing a helper function which studies the
> sg_table and concludes that it's actually
> contiguous.

That indeed sounds like the proper solution for such importers, rather
than making the exporter lie about the buffer bounds to work around
limitations of these importers.

> Did you look at the userspace mmap of TILER
> buffers? I wonder if that goes correctly or not.
> Isn't memory to userspace mapped per page, and
> lengths of the TILER lines are not page aligned?

Mapping to userspace uses an ugly hack whereby small slabs of the
buffer (4096x64 (8bpp), 2048x32 (16bpp), or 1024x32 (32bpp) pixels) are
dynamically mapped to dedicated page-aligned regions of the TILER
virtual space.  For each of the three bitdepths only two such slabs can
be mapped into userspace at any given time (on the entire system), so
using this mechanism to render graphics from userspace can easily cause
hundreds if not thousands of page faults per second.

The alternative (used e.g. in the pyra kernel) is to force all TILER
buffers to be page-aligned, at the cost of wasting some TILER space.
This will presumably also be necessary to allow SGX to import these
buffers since its MMU can obviously also not map data which is not
page-aligned, same for any other importer which uses an MMU to enforce
memory security (rather than being trusted to simply refrain from
accessing data outside the declared bounds).

Ideally such page-alignment should only be applied to buffers which are
intended to be consumed by importers which require this, though it's not
clear how that might be accomplished.

-- 
Matthijs van Duin