Message-ID: <1ce8cea2fdb4169ce827b0bd20d472e697f23c38.camel@mediatek.com>
Date: Thu, 27 Jun 2024 03:17:43 +0000
From: Jason-JH Lin (林睿祥) <Jason-JH.Lin@...iatek.com>
To: "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"matthias.bgg@...il.com" <matthias.bgg@...il.com>, "christian.koenig@....com"
	<christian.koenig@....com>, Yong Wu (吴勇)
	<Yong.Wu@...iatek.com>, "robh+dt@...nel.org" <robh+dt@...nel.org>,
	"sumit.semwal@...aro.org" <sumit.semwal@...aro.org>, "mripard@...nel.org"
	<mripard@...nel.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

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

Powered by blists - more mailing lists