mbox series

[v5,0/9] Add mediate-drm secure flow for SVP

Message ID 20240403102701.369-1-shawn.sung@mediatek.com
Headers show
Series Add mediate-drm secure flow for SVP | expand

Message

Shawn Sung April 3, 2024, 10:26 a.m. UTC
From: Hsiao Chien Sung <shawn.sung@mediatek.corp-partner.google.com>

Memory Definitions:
secure memory - Memory allocated in the TEE (Trusted Execution
Environment) which is inaccessible in the REE (Rich Execution
Environment, i.e. linux kernel/userspace).
secure handle - Integer value which acts as reference to 'secure
memory'. Used in communication between TEE and REE to reference
'secure memory'.
secure buffer - 'secure memory' that is used to store decrypted,
compressed video or for other general purposes in the TEE.
secure surface - 'secure memory' that is used to store graphic buffers.

Memory Usage in SVP:
The overall flow of SVP starts with encrypted video coming in from an
outside source into the REE. The REE will then allocate a 'secure
buffer' and send the corresponding 'secure handle' along with the
encrypted, compressed video data to the TEE. The TEE will then decrypt
the video and store the result in the 'secure buffer'. The REE will
then allocate a 'secure surface'. The REE will pass the 'secure
handles' for both the 'secure buffer' and 'secure surface' into the
TEE for video decoding. The video decoder HW will then decode the
contents of the 'secure buffer' and place the result in the 'secure
surface'. The REE will then attach the 'secure surface' to the overlay
plane for rendering of the video.

Everything relating to ensuring security of the actual contents of the
'secure buffer' and 'secure surface' is out of scope for the REE and
is the responsibility of the TEE.

DRM driver handles allocation of gem objects that are backed by a 'secure
surface' and for displaying a 'secure surface' on the overlay plane.
This introduces a new flag for object creation called
DRM_MTK_GEM_CREATE_ENCRYPTED which indicates it should be a 'secure
surface'. All changes here are in MediaTek specific code.
---
TODO:
1) Remove get sec larb port interface in ddp_comp, ovl and ovl_adaptor.
2) Verify instruction for enabling/disabling dapc and larb port in TEE
   drop the sec_engine flags in normal world and.
3) Move DISP_REG_OVL_SECURE setting to secure world for mtk_disp_ovl.c.
4) Change the parameter register address in mtk_ddp_sec_write()
   from "u32 addr" to "struct cmdq_client_reg *cmdq_reg".
5) Implement setting mmsys routing table in the secure world series.
---
Based on 5 series and 1 patch:
[1] v3 dma-buf: heaps: Add MediaTek secure heap
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=809023
[2] v3 add driver to support secure video decoder
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=807308
[3] v4 soc: mediatek: Add register definitions for GCE
- https://patchwork.kernel.org/project/linux-mediatek/patch/20231212121957.19231-2-shawn.sung@mediatek.com/
[4] v2 Add CMDQ driver support for mt8188
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=810302
[5] Add mediatek,gce-events definition to mediatek,gce-mailbox bindings
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=810938
[6] v3 Add CMDQ secure driver for SVP
- https://patchwork.kernel.org/project/linux-mediatek/list/?series=812379
---
Changes in v5:
1. Sync the local changes

Changes in v4:
1. Rebase on mediatek-drm-next(278640d4d74cd) and fix the conflicts
2. This series is based on 20240129063025.29251-1-yunfei.dong@mediatek.com
3. This series is based on 20240322052829.9893-1-shawn.sung@mediatek.com
4. This series is based on 20240403065603.21920-1-shawn.sung@mediatek.com

Changes in v3:
1. fix kerneldoc problems
2. fix typo in title and commit message
3. adjust naming for secure variable
4. add the missing part for is_suecure plane implementation
5. use BIT_ULL macro to replace bit shifting
6. move modification of ovl_adaptor part to the correct patch
7. add TODO list in commit message
8. add commit message for using share memory to store execute count

Changes in v2:

1. remove the DRIVER_RDNDER flag for mtk_drm_ioctl
2. move cmdq_insert_backup_cookie into client driver
3. move secure gce node define from mt8195-cherry.dtsi to mt8195.dtsi
---
CK Hu (1):
  drm/mediatek: Add interface to allocate MediaTek GEM buffer.

Jason-JH.Lin (9):
  drm/mediatek/uapi: Add DRM_MTK_GEM_CREATE_ENCRYPTED flag
  drm/mediatek: Add secure buffer control flow to mtk_drm_gem
  drm/mediatek: Add secure identify flag and funcution to mtk_drm_plane
  drm/mediatek: Add mtk_ddp_sec_write to config secure buffer info
  drm/mediatek: Add get_sec_port interface to mtk_ddp_comp
  drm/mediatek: Add secure layer config support for ovl
  drm/mediatek: Add secure layer config support for ovl_adaptor
  drm/mediatek: Add secure flow support to mediatek-drm
  drm/mediatek: Add cmdq_insert_backup_cookie before secure pkt finalize

 drivers/gpu/drm/mediatek/mtk_crtc.c           | 275 +++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_crtc.h           |   1 +
 drivers/gpu/drm/mediatek/mtk_ddp_comp.c       |  16 +
 drivers/gpu/drm/mediatek/mtk_ddp_comp.h       |  13 +
 drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   3 +
 drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  30 +-
 .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  15 +
 drivers/gpu/drm/mediatek/mtk_gem.c            |  85 +++++-
 drivers/gpu/drm/mediatek/mtk_gem.h            |   4 +
 drivers/gpu/drm/mediatek/mtk_mdp_rdma.c       |  11 +-
 drivers/gpu/drm/mediatek/mtk_mdp_rdma.h       |   2 +
 drivers/gpu/drm/mediatek/mtk_plane.c          |  25 ++
 drivers/gpu/drm/mediatek/mtk_plane.h          |   2 +
 include/uapi/drm/mediatek_drm.h               |   1 +
 14 files changed, 467 insertions(+), 16 deletions(-)

--
2.18.0

Comments

Maxime Ripard April 15, 2024, 9:32 a.m. UTC | #1
Hi,

On Wed, Apr 03, 2024 at 06:26:53PM +0800, Shawn Sung wrote:
> From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>
> 
> Add DRM_MTK_GEM_CREATE_ENCRYPTED flag to allow user to allocate
> a secure buffer to support secure video path feature.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> ---
>  include/uapi/drm/mediatek_drm.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/drm/mediatek_drm.h b/include/uapi/drm/mediatek_drm.h
> index b0dea00bacbc4..e9125de3a24ad 100644
> --- a/include/uapi/drm/mediatek_drm.h
> +++ b/include/uapi/drm/mediatek_drm.h
> @@ -54,6 +54,7 @@ struct drm_mtk_gem_map_off {
>  
>  #define DRM_MTK_GEM_CREATE		0x00
>  #define DRM_MTK_GEM_MAP_OFFSET		0x01
> +#define DRM_MTK_GEM_CREATE_ENCRYPTED	0x02
>  
>  #define DRM_IOCTL_MTK_GEM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + \
>  		DRM_MTK_GEM_CREATE, struct drm_mtk_gem_create)

That flag doesn't exist in drm-misc-next, which tree is this based on?

Maxime
Maxime Ripard April 15, 2024, 9:37 a.m. UTC | #2
On Wed, Apr 03, 2024 at 06:26:54PM +0800, Shawn Sung wrote:
> From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>
> 
> Add secure buffer control flow to mtk_drm_gem.
> 
> When user space takes DRM_MTK_GEM_CREATE_ENCRYPTED flag and size
> to create a mtk_drm_gem object, mtk_drm_gem will find a matched size
> dma buffer from secure dma-heap and bind it to mtk_drm_gem object.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_gem.c | 85 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_gem.h |  4 ++
>  2 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_gem.c b/drivers/gpu/drm/mediatek/mtk_gem.c
> index e59e0727717b7..ec34d02c14377 100644
> --- a/drivers/gpu/drm/mediatek/mtk_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_gem.c
> @@ -4,6 +4,8 @@
>   */
>  
>  #include <linux/dma-buf.h>
> +#include <linux/dma-heap.h>
> +#include <uapi/linux/dma-heap.h>
>  #include <drm/mediatek_drm.h>
>  
>  #include <drm/drm.h>
> @@ -102,6 +104,81 @@ struct mtk_gem_obj *mtk_gem_create(struct drm_device *dev,
>  	return ERR_PTR(ret);
>  }
>  
> +struct mtk_gem_obj *mtk_gem_create_from_heap(struct drm_device *dev,
> +					     const char *heap, size_t size)
> +{
> +	struct mtk_drm_private *priv = dev->dev_private;
> +	struct mtk_gem_obj *mtk_gem;
> +	struct drm_gem_object *obj;
> +	struct dma_heap *dma_heap;
> +	struct dma_buf *dma_buf;
> +	struct dma_buf_attachment *attach;
> +	struct sg_table *sgt;
> +	struct iosys_map map = {};
> +	int ret;
> +
> +	mtk_gem = mtk_gem_init(dev, size);
> +	if (IS_ERR(mtk_gem))
> +		return ERR_CAST(mtk_gem);
> +
> +	obj = &mtk_gem->base;
> +
> +	dma_heap = dma_heap_find(heap);
> +	if (!dma_heap) {
> +		DRM_ERROR("heap find fail\n");
> +		goto err_gem_free;
> +	}
> +	dma_buf = dma_heap_buffer_alloc(dma_heap, size,
> +					O_RDWR | O_CLOEXEC, DMA_HEAP_VALID_HEAP_FLAGS);
> +	if (IS_ERR(dma_buf)) {
> +		DRM_ERROR("buffer alloc fail\n");
> +		dma_heap_put(dma_heap);
> +		goto err_gem_free;
> +	}
> +	dma_heap_put(dma_heap);
> +
> +	attach = dma_buf_attach(dma_buf, priv->dma_dev);
> +	if (IS_ERR(attach)) {
> +		DRM_ERROR("attach fail, return\n");
> +		dma_buf_put(dma_buf);
> +		goto err_gem_free;
> +	}
> +
> +	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
> +	if (IS_ERR(sgt)) {
> +		DRM_ERROR("map failed, detach and return\n");
> +		dma_buf_detach(dma_buf, attach);
> +		dma_buf_put(dma_buf);
> +		goto err_gem_free;
> +	}
> +	obj->import_attach = attach;
> +	mtk_gem->dma_addr = sg_dma_address(sgt->sgl);
> +	mtk_gem->sg = sgt;
> +	mtk_gem->size = dma_buf->size;
> +
> +	if (!strcmp(heap, "mtk_svp") || !strcmp(heap, "mtk_svp_cma")) {
> +		/* secure buffer can not be mapped */
> +		mtk_gem->secure = true;
> +	} else {
> +		ret = dma_buf_vmap(dma_buf, &map);
> +		mtk_gem->kvaddr = map.vaddr;
> +		if (ret) {
> +			DRM_ERROR("map failed, ret=%d\n", ret);
> +			dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
> +			dma_buf_detach(dma_buf, attach);
> +			dma_buf_put(dma_buf);
> +			mtk_gem->kvaddr = NULL;
> +		}
> +	}
> +
> +	return mtk_gem;
> +
> +err_gem_free:
> +	drm_gem_object_release(obj);
> +	kfree(mtk_gem);
> +	return ERR_PTR(-ENOMEM);
> +}
> +
>  void mtk_gem_free_object(struct drm_gem_object *obj)
>  {
>  	struct mtk_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> @@ -229,7 +306,9 @@ struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev,
>  	if (IS_ERR(mtk_gem))
>  		return ERR_CAST(mtk_gem);
>  
> +	mtk_gem->secure = !sg_page(sg->sgl);
>  	mtk_gem->dma_addr = sg_dma_address(sg->sgl);
> +	mtk_gem->size = attach->dmabuf->size;
>  	mtk_gem->sg = sg;
>  
>  	return &mtk_gem->base;
> @@ -304,7 +383,11 @@ int mtk_gem_create_ioctl(struct drm_device *dev, void *data,
>  	struct drm_mtk_gem_create *args = data;
>  	int ret;
>  
> -	mtk_gem = mtk_gem_create(dev, args->size, false);
> +	if (args->flags & DRM_MTK_GEM_CREATE_ENCRYPTED)
> +		mtk_gem = mtk_gem_create_from_heap(dev, "mtk_svp_cma", args->size);

That heap doesn't exist upstream either. Also, I'm wondering if it's the
right solution there.

From what I can tell, you want to allow to create encrypted buffers from
the TEE. Why do we need this as a DRM ioctl at all? A heap seems like
the perfect solution to do so, and then you just have to import it into
DRM.

I'm also not entirely sure that not having a SG list is enough to
consider the buffer secure. Wouldn't a buffer allocated without a kernel
mapping also be in that situation?

Maxime
Jeffrey Kardatzke April 16, 2024, 5:19 p.m. UTC | #3
I would argue 'restricted' is the proper name since that was what was
settled on for the dma-buf code.  :)  But you are definitely right
that this memory is not encrypted.


On Tue, Apr 16, 2024 at 7:09 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi,
>
> Le mercredi 03 avril 2024 à 18:26 +0800, Shawn Sung a écrit :
> > From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>
> >
> > Add DRM_MTK_GEM_CREATE_ENCRYPTED flag to allow user to allocate
>
> Is "ENCRYPTED" a proper naming ? My expectation is that this would hold data in
> a PROTECTED memory region but that no cryptographic algorithm will be involved.
>
> Nicolas
>
> > a secure buffer to support secure video path feature.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > ---
> >  include/uapi/drm/mediatek_drm.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/uapi/drm/mediatek_drm.h b/include/uapi/drm/mediatek_drm.h
> > index b0dea00bacbc4..e9125de3a24ad 100644
> > --- a/include/uapi/drm/mediatek_drm.h
> > +++ b/include/uapi/drm/mediatek_drm.h
> > @@ -54,6 +54,7 @@ struct drm_mtk_gem_map_off {
> >
> >  #define DRM_MTK_GEM_CREATE           0x00
> >  #define DRM_MTK_GEM_MAP_OFFSET               0x01
> > +#define DRM_MTK_GEM_CREATE_ENCRYPTED 0x02
> >
> >  #define DRM_IOCTL_MTK_GEM_CREATE     DRM_IOWR(DRM_COMMAND_BASE + \
> >               DRM_MTK_GEM_CREATE, struct drm_mtk_gem_create)
>
>
Jason-JH.Lin April 17, 2024, 8:36 a.m. UTC | #4
On Mon, 2024-04-15 at 11:32 +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Apr 03, 2024 at 06:26:53PM +0800, Shawn Sung wrote:
> > From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>
> > 
> > Add DRM_MTK_GEM_CREATE_ENCRYPTED flag to allow user to allocate
> > a secure buffer to support secure video path feature.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > ---
> >  include/uapi/drm/mediatek_drm.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/uapi/drm/mediatek_drm.h
> > b/include/uapi/drm/mediatek_drm.h
> > index b0dea00bacbc4..e9125de3a24ad 100644
> > --- a/include/uapi/drm/mediatek_drm.h
> > +++ b/include/uapi/drm/mediatek_drm.h
> > @@ -54,6 +54,7 @@ struct drm_mtk_gem_map_off {
> >  
> >  #define DRM_MTK_GEM_CREATE		0x00
> >  #define DRM_MTK_GEM_MAP_OFFSET		0x01
> > +#define DRM_MTK_GEM_CREATE_ENCRYPTED	0x02
> >  
> >  #define DRM_IOCTL_MTK_GEM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + \
> >  		DRM_MTK_GEM_CREATE, struct drm_mtk_gem_create)
> 
> That flag doesn't exist in drm-misc-next, which tree is this based
> on?
> 
I think we missed the patch [1] in this series.
[1] 
https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102602.32155-11-shawn.sung@mediatek.com/

I'll add it back at the next version.

Regards,
Jason-JH.Lin

> Maxime
Jason-JH.Lin May 28, 2024, 7:06 a.m. UTC | #5
Hi Maxime,

[snip]

I'm sorry for losing your previous comment mail.
I finally found a way to import this mail back so I can reply to you.

> > -	mtk_gem = mtk_gem_create(dev, args->size, false);
> > +	if (args->flags & DRM_MTK_GEM_CREATE_ENCRYPTED)
> > +		mtk_gem = mtk_gem_create_from_heap(dev, "mtk_svp_cma",
> > args->size);
> 
> That heap doesn't exist upstream either. Also, I'm wondering if it's
> the
> right solution there.
> 

Yes, I found that its name changed to "restricted_mtk_cma" in the
latest patch: 
https://patchwork.kernel.org/project/linux-mediatek/patch/20240515112308.10171-10-yong.wu@mediatek.com/

> From what I can tell, you want to allow to create encrypted buffers
> from
> the TEE. Why do we need this as a DRM ioctl at all? A heap seems like
> the perfect solution to do so, and then you just have to import it
> into
> DRM.
> 

OK, I'll try to change the userspace's ioctl from
DRM_IOCTL_MTK_GEM_CREATE to DMA_HEAP_IOCTL_ALLOC to get the buffer fd,
then import to DRM.

> I'm also not entirely sure that not having a SG list is enough to
> consider the buffer secure. Wouldn't a buffer allocated without a
> kernel
> mapping also be in that situation?
> 

I have confirmed to Yong.Wu that secure buffer also have sg list, so
the secure checking method "!sg_page(sg->sgl)" will be deprecated.

Regards,
Jason-JH.Lin

> Maxime
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>