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

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

On Wed, 2024-06-26 at 12:49 +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 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥):
> > > 
> > > > 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?
> > 
> > I need to know the imported buffer is allocated from restricted cma
> > and
> > mark it as a secure buffer in mediatek-drm driver. Then, I will add
> > some configuration to the hardware if the buffer is secure buffer,
> > so
> > that it can get the permission to access the secure buffer.
>  
> Yeah so far that makes sense. This is basically what other drivers do
> with secure buffers as well.
> 
> But why do you want the kernel to transport that information? Usually
> drivers get the information from userspace what to do with a buffer.
> 
> In other words the format, stride, tilling and also if it's a secure
> buffer or not comes from userspace.
> 

Thanks for your clear explanation.
I think this is what I want, but I can't find any DRM interface to pass
the secure flag from userspace.

> What the hardware usually handles internally is things like
> encryption keys, but you eventually get the information where to look
> for the key from userspace as well.
> 
> Handling inside the kernel would only be necessary if userspace could
> for example crash the system with invalid parameters. But for
> encryption that is usually not the case.
> 

Yes, that's true.

> > > 
> > > > 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.
> > > 
> > 
> > You mean I should allocate buffer in mediate-drm driver not
> > userspace?
>  
> Well that depends. The question is if you have multiple drivers which
> needs to work with this secure buffer?
> 
> If yes then you should have a general allocation heap for it. If no
> then the buffers could as well be allocated from the driver interface
> directly.
> 

Yes, this buffer needs work with GPU and DRM drivers, so this general
"restricted_mtk_cma" will allocated in userspace, then being passed to
GPU and DRM.

> > I just have modified this to userspace by the comment here:
> > 
> > 
https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.369-3-shawn.sung@mediatek.com/#25806766
> > 
> > > > 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?
> > 
> > We use the same mediatek-drm driver for secure and non-secure
> > buffer.
> > If non-secure buffer imported to mediatek-drm driver, it's go to
> > the
> > normal flow with normal hardware settings.
> > 
> > We use different configurations to make hardware have different
> > permission to access the buffer it should access.
> > 
> > So if we can't get the information of "the buffer is allocated from
> > restricted_mtk_cma" when importing the buffer into the driver, we
> > won't
> > be able to configure the hardware correctly.
>  
> Why can't you get this information from userspace?

As I mentioned here: 

https://patchwork.kernel.org/project/linux-mediatek/cover/20240525232928.5524-1-jason-jh.lin@mediatek.com/#25886488

I tried some DRM interfaces using buffer FD and arg->flag as
parameters, but it didn't work. So I ask for your help here.

But I think I should find DRM maintainer to add the secure flag to DRM
interface now.

Regards,
Jason-JH.Lin