Message-ID: <1050c44512374031d1349b5dced228d0efc3fbde.camel@mediatek.com>
Date: Tue, 25 Jun 2024 11:02:36 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: "robh+dt@...nel.org" <robh+dt@...nel.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "christian.koenig@....com"
	<christian.koenig@....com>, Yong Wu (吴勇)
	<Yong.Wu@...iatek.com>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>, "mripard@...nel.org" <mripard@...nel.org>,
	"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mediatek@...ts.infradead.org" <linux-mediatek@...ts.infradead.org>,
	"jstultz@...gle.com" <jstultz@...gle.com>, "linaro-mm-sig@...ts.linaro.org"
	<linaro-mm-sig@...ts.linaro.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>,
	Jianjiao Zeng (曾健姣)
	<Jianjiao.Zeng@...iatek.com>, "willy@...radead.org" <willy@...radead.org>,
	Kuohong Wang (王國鴻) <kuohong.wang@...iatek.com>,
	"quic_vjitta@...cinc.com" <quic_vjitta@...cinc.com>, "pavel@....cz"
	<pavel@....cz>, "robin.murphy@....com" <robin.murphy@....com>,
	"contact@...rsion.fr" <contact@...rsion.fr>, "logang@...tatee.com"
	<logang@...tatee.com>, "daniel@...ll.ch" <daniel@...ll.ch>,
	"jkardatzke@...gle.com" <jkardatzke@...gle.com>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "Brian.Starkey@....com" <Brian.Starkey@....com>,
	"benjamin.gaignard@...labora.com" <benjamin.gaignard@...labora.com>,
	"tjmercier@...gle.com" <tjmercier@...gle.com>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "joakim.bech@...aro.org"
	<joakim.bech@...aro.org>, "ppaalanen@...il.com" <ppaalanen@...il.com>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>,
	Youlin Pei (裴友林) <youlin.pei@...iatek.com>
Subject: Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

Hi Christian,

On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote:
> Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇):
> > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote:
> > >   	
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >   Am 15.05.24 um 13:23 schrieb Yong Wu:
> > > > Introduce a FLAG for the restricted memory which means the
> > > > memory
> > > 
> > > is
> > > > protected by TEE or hypervisor, then it's inaccessiable for
> > > > kernel.
> > > > 
> > > > Currently we don't use sg_dma_unmark_restricted, thus this
> > > 
> > > interface
> > > > has not been added.
> > > 
> > > Why should that be part of the scatterlist? It doesn't seem to
> > > affect
> > > any of it's functionality.
> > > 
> > > As far as I can see the scatterlist shouldn't be the transport of
> > > this
> > > kind of information.
> > 
> > Thanks for the review. I will remove this.
> > 
> > In our user scenario, DRM will import these buffers and check if
> > this
> > is a restricted buffer. If yes, it will use secure GCE takes over.
> > 
> > If this judgment is not suitable to be placed in scatterlist. I
> > don't
> > know if it is ok to limit this inside dma-buf. Adding such an
> > interface:
> > 
> > static bool dma_buf_is_restricted(struct dma_buf *dmabuf)
> > {
> > 	return !strncmp(dmabuf->exp_name, "restricted", 10);
> > }
> 
> No, usually stuff like that doesn't belong into DMA buf either.
> 
> Question here really is who controls the security status of the
> memory 
> backing the buffer?
> 
> In other words who tells the exporter that it should allocate and
> fill a 
> buffer with encrypted data?
> 
> If that is userspace then that is part of the format information and
> it 
> is also userspace who should tell the importer that it needs to work 
> with encrypted data.
> 
> The kernel is intentionally not involved in stuff like that.
> 

Here is the expected protected content buffer flow in DRM:
1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by
DMA_HEAP_IOCTL_ALLOC.
2) userspace imports that dma-buf into the device using prime for the
drm_file.
3) userspace uses the already implemented driver import code for the
special cases of protected content buffer.

In the step 3), we need to verify the dma-buf is allocated from
"restricted_mtk_cma", but there is no way to pass the secure flag or
private data from userspace to the import interface in DRM driver.

So I can only verify it like this now:
struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device
	*dev, struct dma_buf_attachment *attach, struct sg_table *sg)
{
    struct mtk_gem_obj *mtk_gem;

    /* check if the entries in the sg_table are contiguous */
    if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
        DRM_ERROR("sg_table is not contiguous");
        return ERR_PTR(-EINVAL);
    }
    mtk_gem = mtk_gem_init(dev, attach->dmabuf->size);
    if (IS_ERR(mtk_gem))
        return ERR_CAST(mtk_gem);

+   mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted",
10));
    mtk_gem->dma_addr = sg_dma_address(sg->sgl);
    mtk_gem->size = attach->dmabuf->size;
    mtk_gem->sg = sg;

    return &mtk_gem->base;
}

I think I have the same problem as the ECC_FLAG mention in:

https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd049511@kernel.org/

I think it would be better to have the user configurable private
information in dma-buf, so all the drivers who have the same
requirement can get their private information from dma-buf directly and
no need to change or add the interface.

What's your opinion in this point?

Regards,
Jason-JH.Lin

> Regards,
> Christian.

Powered by blists - more mailing lists