Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

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.