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

[Christian König]

Am 25.06.24 um 13:02 schrieb Jason-JH Lin (林睿祥):
>
> 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.

What is so special on that case?

>
> 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.

Why do you need to verify that?

>
> 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;
> }

Complete NAK from my side to that approach. Importing of a DMA-buf 
should be independent of the exporter.

What you could do is to provide the secure buffer from a device and not 
a device heap.

> 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?

Well of hand I don't see the need for that.

What happens if you get a non-secure buffer imported in your secure device?

Regards,
Christian.

>
> Regards,
> Jason-JH.Lin
>
> > Regards,
> > Christian.
>
> ************* MEDIATEK Confidentiality Notice
>   ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
>   
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240625/88164a95/attachment-0001.htm>