Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[Daniel Stone]

Hi Jason, CK,

On Tue, 19 Sept 2023 at 04:04, Jason-JH.Lin <jason-jh.lin@mediatek.com> wrote:
> The patch series provides drm driver support for enabling secure video
> path (SVP) playback on MediaiTek hardware in the Linux kernel.
>
> [...]
>
> 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.

To be honest, it seems strange that DRM is being used as the allocator
for buffers which will mostly be used by codec hardware which is not
mentioned here. I can understand that minigbm and gbm_gralloc make
this easy to implement, but it's not really right to add this all to
mtk-drm just to support some codec features.

NXP posted a patchset a while ago to add secure-memory support to
dma-heaps[0]. This would be much cleaner (e.g. avoiding strcmp on
allocating name, avoiding mtk-drm being a render node when it can't
render) I think, and also allow common secure-path semantics between
different vendors.

Having common secure-path semantics between different vendors would be
very helpful, because the first question when we add new uAPI is
'where is the open-source userspace?'. If there is at least a common
interface through e.g. dma-heaps, then we could have some standard
cross-vendor userspace code which would work well with the standard
interface.

Cheers,
Daniel

[0]: https://lore.kernel.org/lkml/20220805135330.970-2-olivier.masse@nxp.com/

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[Jeffrey Kardatzke]

On Mon, Sep 18, 2023 at 11:33 PM Daniel Stone <daniel@fooishbar.org> wrote:
>
> Hi Jason, CK,
>
> On Tue, 19 Sept 2023 at 04:04, Jason-JH.Lin <jason-jh.lin@mediatek.com> wrote:
> > The patch series provides drm driver support for enabling secure video
> > path (SVP) playback on MediaiTek hardware in the Linux kernel.
> >
> > [...]
> >
> > 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.
>
> To be honest, it seems strange that DRM is being used as the allocator
> for buffers which will mostly be used by codec hardware which is not
> mentioned here. I can understand that minigbm and gbm_gralloc make
> this easy to implement, but it's not really right to add this all to
> mtk-drm just to support some codec features.
The buffers allocated are used as the output for secure video decoding
and then rendering as well. So they aren't just used by the codec HW,
they're also used for display on the overlay plane.

And I'm the user of all the secure video path patches Mediatek has
been posting, this is for secure video playback on ChromeOS ARM
devices. (just mentioning so you know my context in this all)
>
> NXP posted a patchset a while ago to add secure-memory support to
> dma-heaps[0]. This would be much cleaner (e.g. avoiding strcmp on
> allocating name, avoiding mtk-drm being a render node when it can't
> render) I think, and also allow common secure-path semantics between
> different vendors.
>
Yes, I saw that now. I agree that having a common secure-path solution
is preferable. But the two issues you mention with this patchset
aren't directly related to the dma-buf heap implementation I think.

The ugly strcmp can be removed from this patchset...because it doesn't
actually need to check the heap name, it only ever invokes
mtk_drm_gem_create_from_heap for secure memory allocations...so that
should just be renamed mtk_drm_gem_create_from_secure_heap. The RENDER
flag can also be removed. IIUC, that's an artifact of how ChromeOS is
doing the allocations and that it's performing it on a render node.
That can be removed from this patchset and we can address that problem
on the ChromeOS side instead.

Going back to the NXP implementation...the main difference between
that one and what Mediatek is proposing (aside from all their vendor
specific naming of things) is that the NXP patchset does all the
allocation in the kernel itself and the kernel is handing out physical
addresses from the reserved range w/ no virtual address mapping. I
think that would mean you have to always allocate contiguous blocks,
which would be prone to fragmentation.  In the Mediatek
implementation, they are doing the allocations from the heap in the
TEE, so they can deal with physical memory fragmentation through
virtual addresses since they can do SG lists to allocate from
fragmented memory in the TEE.




> Having common secure-path semantics between different vendors would be
> very helpful, because the first question when we add new uAPI is
> 'where is the open-source userspace?'. If there is at least a common
> interface through e.g. dma-heaps, then we could have some standard
> cross-vendor userspace code which would work well with the standard
> interface.
>
Thanks for your feedback, I definitely want to work to get this to
something more usable by a wider range.

> Cheers,
> Daniel
>
> [0]: https://lore.kernel.org/lkml/20220805135330.970-2-olivier.masse@nxp.com/

Re: [PATCH 10/10] arm64: dts: mt8195-cherry: Add secure mbox settings for vdosys

[AngeloGioacchino Del Regno]

Il 19/09/23 05:03, Jason-JH.Lin ha scritto:
> Add a secure mailbox channel to support secure video path on
> vdosys0 and vdosys1.
> 
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> index 37a3e9de90ff..9b838b2cdb3e 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
> @@ -1147,6 +1147,16 @@
>   	status = "okay";
>   };
>   
> +&vdosys0 {
> +	mboxes = <&gce0 0 CMDQ_THR_PRIO_4>,
> +		 <&gce0 8 CMDQ_THR_PRIO_4>; /* secure mbox */
> +};
> +
> +&vdosys1 {
> +	mboxes = <&gce0 1 CMDQ_THR_PRIO_4>,
> +		 <&gce0 9 CMDQ_THR_PRIO_4>; /* secure mbox */
> +};
> +

Is the secure mailbox number firmware dependant?
Otherwise this could go in mt8195.dtsi.

Regards,
Angelo

>   &xhci0 {
>   	status = "okay";
>   

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[CK Hu (胡俊光)]

Hi, Jason:

On Tue, 2023-09-19 at 11:03 +0800, Jason-JH.Lin wrote:
> The patch series provides drm driver support for enabling secure
> video
> path (SVP) playback on MediaiTek hardware in the Linux kernel.
> 
> 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.

How do you define SVP? Is there standard requirement we could refer to?
If the secure video buffer is read by display hardware and output to
HDMI without any protection and user could capture HDMI signal, is this
secure?

Regards,
CK

> 
> ---
> Based on 2 series:
> [1] Add CMDQ secure driver for SVP
> - 
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=785332__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj1ltVnhGY$
>  
> 
> [2] dma-buf: heaps: Add MediaTek secure heap
> - 
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj10sD4kHE$
>  
> ---
> 
> CK Hu (1):
>   drm/mediatek: Add interface to allocate MediaTek GEM buffer.
> 
> Jason-JH.Lin (9):
>   drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED 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
>   arm64: dts: mt8195-cherry: Add secure mbox settings for vdosys
> 
>  .../boot/dts/mediatek/mt8195-cherry.dtsi      |  10 +
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   3 +
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  31 +-
>  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  15 +
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 271
> +++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.h       |   1 +
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  14 +
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  13 +
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  16 +-
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c        | 121 ++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_gem.h        |  16 ++
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c      |   7 +
>  drivers/gpu/drm/mediatek/mtk_drm_plane.h      |   2 +
>  drivers/gpu/drm/mediatek/mtk_mdp_rdma.c       |  11 +-
>  drivers/gpu/drm/mediatek/mtk_mdp_rdma.h       |   2 +
>  include/uapi/drm/mediatek_drm.h               |  59 ++++
>  16 files changed, 575 insertions(+), 17 deletions(-)
>  create mode 100644 include/uapi/drm/mediatek_drm.h
> 

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[Jeffrey Kardatzke]

On Tue, Sep 19, 2023 at 10:26 PM CK Hu (胡俊光) <ck.hu@mediatek.com> wrote:
>
> Hi, Jason:
>
> On Tue, 2023-09-19 at 11:03 +0800, Jason-JH.Lin wrote:
> > The patch series provides drm driver support for enabling secure
> > video
> > path (SVP) playback on MediaiTek hardware in the Linux kernel.
> >
> > 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.
>
> How do you define SVP? Is there standard requirement we could refer to?
> If the secure video buffer is read by display hardware and output to
> HDMI without any protection and user could capture HDMI signal, is this
> secure?

SVP (Secure Video Path) is essentially the video being completed
isolated from the kernel/userspace. The specific requirements for it
vary between implementations.

Regarding HDMI/HDCP output; it's the responsibility of the TEE to
enforce that. Nothing on the kernel/userspace side needs to be
concerned about enforcing HDCP. The only thing userspace is involved
in there is actually turning on HDCP via the kernel drivers; and then
the TEE ensures that it is active if the policy for the encrypted
content requires it.
>
> Regards,
> CK
>
> >
> > ---
> > Based on 2 series:
> > [1] Add CMDQ secure driver for SVP
> > -
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=785332__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj1ltVnhGY$
> >
> >
> > [2] dma-buf: heaps: Add MediaTek secure heap
> > -
> > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj10sD4kHE$
> >
> > ---
> >
> > CK Hu (1):
> >   drm/mediatek: Add interface to allocate MediaTek GEM buffer.
> >
> > Jason-JH.Lin (9):
> >   drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED 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
> >   arm64: dts: mt8195-cherry: Add secure mbox settings for vdosys
> >
> >  .../boot/dts/mediatek/mt8195-cherry.dtsi      |  10 +
> >  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   3 +
> >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  31 +-
> >  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  15 +
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 271
> > +++++++++++++++++-
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h       |   1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  14 +
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  13 +
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  16 +-
> >  drivers/gpu/drm/mediatek/mtk_drm_gem.c        | 121 ++++++++
> >  drivers/gpu/drm/mediatek/mtk_drm_gem.h        |  16 ++
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c      |   7 +
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.h      |   2 +
> >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.c       |  11 +-
> >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.h       |   2 +
> >  include/uapi/drm/mediatek_drm.h               |  59 ++++
> >  16 files changed, 575 insertions(+), 17 deletions(-)
> >  create mode 100644 include/uapi/drm/mediatek_drm.h
> >

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[CK Hu (胡俊光)]

On Wed, 2023-09-20 at 11:25 -0700, Jeffrey Kardatzke wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Tue, Sep 19, 2023 at 10:26 PM CK Hu (胡俊光) <ck.hu@mediatek.com>
> wrote:
> >
> > Hi, Jason:
> >
> > On Tue, 2023-09-19 at 11:03 +0800, Jason-JH.Lin wrote:
> > > The patch series provides drm driver support for enabling secure
> > > video
> > > path (SVP) playback on MediaiTek hardware in the Linux kernel.
> > >
> > > 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.
> >
> > How do you define SVP? Is there standard requirement we could refer
> to?
> > If the secure video buffer is read by display hardware and output
> to
> > HDMI without any protection and user could capture HDMI signal, is
> this
> > secure?
> 
> SVP (Secure Video Path) is essentially the video being completed
> isolated from the kernel/userspace. The specific requirements for it
> vary between implementations.
> 
> Regarding HDMI/HDCP output; it's the responsibility of the TEE to
> enforce that. Nothing on the kernel/userspace side needs to be
> concerned about enforcing HDCP. The only thing userspace is involved
> in there is actually turning on HDCP via the kernel drivers; and then
> the TEE ensures that it is active if the policy for the encrypted
> content requires it.

But in [Patch 07/10],

in mtk_ovl_layer_config(), the secure input would enable/disable
dynamical in kernel, MediaTek SVP does not hide all control in TEE, so
I worry that something lost. To achieve SVP, display hardware should
have secure input, secure pipeline, secure output, but in this series,
I just see secure input. Maybe others is done in TEE, tell me which
part is done by which step.

Regards,
CK


> >
> > Regards,
> > CK
> >
> > >
> > > ---
> > > Based on 2 series:
> > > [1] Add CMDQ secure driver for SVP
> > > -
> > > 
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=785332__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj1ltVnhGY$
> > >
> > >
> > > [2] dma-buf: heaps: Add MediaTek secure heap
> > > -
> > > 
> https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj10sD4kHE$
> > >
> > > ---
> > >
> > > CK Hu (1):
> > >   drm/mediatek: Add interface to allocate MediaTek GEM buffer.
> > >
> > > Jason-JH.Lin (9):
> > >   drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED 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
> > >   arm64: dts: mt8195-cherry: Add secure mbox settings for vdosys
> > >
> > >  .../boot/dts/mediatek/mt8195-cherry.dtsi      |  10 +
> > >  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   3 +
> > >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  31 +-
> > >  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  15 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 271
> > > +++++++++++++++++-
> > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h       |   1 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  14 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  13 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  16 +-
> > >  drivers/gpu/drm/mediatek/mtk_drm_gem.c        | 121 ++++++++
> > >  drivers/gpu/drm/mediatek/mtk_drm_gem.h        |  16 ++
> > >  drivers/gpu/drm/mediatek/mtk_drm_plane.c      |   7 +
> > >  drivers/gpu/drm/mediatek/mtk_drm_plane.h      |   2 +
> > >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.c       |  11 +-
> > >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.h       |   2 +
> > >  include/uapi/drm/mediatek_drm.h               |  59 ++++
> > >  16 files changed, 575 insertions(+), 17 deletions(-)
> > >  create mode 100644 include/uapi/drm/mediatek_drm.h
> > >
> 

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[Jason-JH Lin (林睿祥)]

Hi CK,


On Thu, 2023-09-21 at 01:31 +0000, CK Hu (胡俊光) wrote:
> On Wed, 2023-09-20 at 11:25 -0700, Jeffrey Kardatzke wrote:
> >  	 
> > External email : Please do not click links or open attachments
> > until
> > you have verified the sender or the content.
> >  On Tue, Sep 19, 2023 at 10:26 PM CK Hu (胡俊光) <ck.hu@mediatek.com>
> > wrote:
> > > 
> > > Hi, Jason:
> > > 
> > > On Tue, 2023-09-19 at 11:03 +0800, Jason-JH.Lin wrote:
> > > > The patch series provides drm driver support for enabling
> > > > secure
> > > > video
> > > > path (SVP) playback on MediaiTek hardware in the Linux kernel.
> > > > 
> > > > 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.
> > > 
> > > How do you define SVP? Is there standard requirement we could
> > > refer
> > 
> > to?
> > > If the secure video buffer is read by display hardware and output
> > 
> > to
> > > HDMI without any protection and user could capture HDMI signal,
> > > is
> > 
> > this
> > > secure?
> > 
> > SVP (Secure Video Path) is essentially the video being completed
> > isolated from the kernel/userspace. The specific requirements for
> > it
> > vary between implementations.
> > 
> > Regarding HDMI/HDCP output; it's the responsibility of the TEE to
> > enforce that. Nothing on the kernel/userspace side needs to be
> > concerned about enforcing HDCP. The only thing userspace is
> > involved
> > in there is actually turning on HDCP via the kernel drivers; and
> > then
> > the TEE ensures that it is active if the policy for the encrypted
> > content requires it.
> 
> But in [Patch 07/10],
> 
> in mtk_ovl_layer_config(), the secure input would enable/disable
> dynamical in kernel, MediaTek SVP does not hide all control in TEE,
> so
> I worry that something lost. To achieve SVP, display hardware should
> have secure input, secure pipeline, secure output, but in this
> series,
> I just see secure input. Maybe others is done in TEE, tell me which
> part is done by which step.
> 
We'll use cmdq_sec_pkt_set_data() to bring the secure scenario, secure
engine flags and some secure metadata to TEE world. Then will used
these information to make sure the pipeline is secure.

We don't have the secure output feature currently, such as WiFi
display, so we'll do it after we have to support the feature.

Also there are HDMITX_HDCP and DPTX_HDCP TA will check the HDCP statue
in secure world and then CMDQ TA will get that status by calling their
API in TEE.
If CMDQ TA get a HDCP checking failed sstatus, it will insert some
instructions in the secure cmd buffer to mute the DISP HW for each
frames.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> 
> > > 
> > > Regards,
> > > CK
> > > 
> > > > 
> > > > ---
> > > > Based on 2 series:
> > > > [1] Add CMDQ secure driver for SVP
> > > > -
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=785332__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj1ltVnhGY$
> > > > 
> > > > 
> > > > [2] dma-buf: heaps: Add MediaTek secure heap
> > > > -
> > > > 
> > 
> > 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj10sD4kHE$
> > > > 
> > > > ---
> > > > 
> > > > CK Hu (1):
> > > >   drm/mediatek: Add interface to allocate MediaTek GEM buffer.
> > > > 
> > > > Jason-JH.Lin (9):
> > > >   drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED 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
> > > >   arm64: dts: mt8195-cherry: Add secure mbox settings for
> > > > vdosys
> > > > 
> > > >  .../boot/dts/mediatek/mt8195-cherry.dtsi      |  10 +
> > > >  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   3 +
> > > >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  31 +-
> > > >  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  15 +
> > > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 271
> > > > +++++++++++++++++-
> > > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h       |   1 +
> > > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  14 +
> > > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  13 +
> > > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  16 +-
> > > >  drivers/gpu/drm/mediatek/mtk_drm_gem.c        | 121 ++++++++
> > > >  drivers/gpu/drm/mediatek/mtk_drm_gem.h        |  16 ++
> > > >  drivers/gpu/drm/mediatek/mtk_drm_plane.c      |   7 +
> > > >  drivers/gpu/drm/mediatek/mtk_drm_plane.h      |   2 +
> > > >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.c       |  11 +-
> > > >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.h       |   2 +
> > > >  include/uapi/drm/mediatek_drm.h               |  59 ++++
> > > >  16 files changed, 575 insertions(+), 17 deletions(-)
> > > >  create mode 100644 include/uapi/drm/mediatek_drm.h
> > > > 

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[CK Hu (胡俊光)]

Hi, Jason:

On Thu, 2023-09-21 at 14:55 +0000, Jason-JH Lin (林睿祥) wrote:
> Hi CK,
> 
> 
> On Thu, 2023-09-21 at 01:31 +0000, CK Hu (胡俊光) wrote:
> > On Wed, 2023-09-20 at 11:25 -0700, Jeffrey Kardatzke wrote:
> > >  	 
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >  On Tue, Sep 19, 2023 at 10:26 PM CK Hu (胡俊光) <ck.hu@mediatek.com
> > > >
> > > wrote:
> > > > 
> > > > Hi, Jason:
> > > > 
> > > > On Tue, 2023-09-19 at 11:03 +0800, Jason-JH.Lin wrote:
> > > > > The patch series provides drm driver support for enabling
> > > > > secure
> > > > > video
> > > > > path (SVP) playback on MediaiTek hardware in the Linux
> > > > > kernel.
> > > > > 
> > > > > 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.
> > > > 
> > > > How do you define SVP? Is there standard requirement we could
> > > > refer
> > > 
> > > to?
> > > > If the secure video buffer is read by display hardware and
> > > > output
> > > 
> > > to
> > > > HDMI without any protection and user could capture HDMI signal,
> > > > is
> > > 
> > > this
> > > > secure?
> > > 
> > > SVP (Secure Video Path) is essentially the video being completed
> > > isolated from the kernel/userspace. The specific requirements for
> > > it
> > > vary between implementations.
> > > 
> > > Regarding HDMI/HDCP output; it's the responsibility of the TEE to
> > > enforce that. Nothing on the kernel/userspace side needs to be
> > > concerned about enforcing HDCP. The only thing userspace is
> > > involved
> > > in there is actually turning on HDCP via the kernel drivers; and
> > > then
> > > the TEE ensures that it is active if the policy for the encrypted
> > > content requires it.
> > 
> > But in [Patch 07/10],
> > 
> > in mtk_ovl_layer_config(), the secure input would enable/disable
> > dynamical in kernel, MediaTek SVP does not hide all control in TEE,
> > so
> > I worry that something lost. To achieve SVP, display hardware
> > should
> > have secure input, secure pipeline, secure output, but in this
> > series,
> > I just see secure input. Maybe others is done in TEE, tell me which
> > part is done by which step.
> > 
> 
> We'll use cmdq_sec_pkt_set_data() to bring the secure scenario,
> secure
> engine flags and some secure metadata to TEE world. Then will used
> these information to make sure the pipeline is secure.
> 
> We don't have the secure output feature currently, such as WiFi
> display, so we'll do it after we have to support the feature.
> 
> Also there are HDMITX_HDCP and DPTX_HDCP TA will check the HDCP
> statue
> in secure world and then CMDQ TA will get that status by calling
> their
> API in TEE.
> If CMDQ TA get a HDCP checking failed sstatus, it will insert some
> instructions in the secure cmd buffer to mute the DISP HW for each
> frames.

You enable secure path by crtc pipeline. That means it may be primary
display is secure and secondary display is non-secure. In
drivers/soc/mediatek/mt8195-mmsys.h, the primary display input could be
routed to secondary display output. Is mmsys protected when display is
secure? The whole mmsys is protected or partial mmsys is protected? If
the whole mmsys is protected, the non-secure display pipeline would be
malfunctioned because the control of non-secure display is in normal
world and it may access mmsys. Please describe how this work?

Regards,
CK

> 
> Regards,
> Jason-JH.Lin
> 
> > Regards,
> > CK
> > 
> > 
> > > > 
> > > > Regards,
> > > > CK
> > > > 
> > > > > 
> > > > > ---
> > > > > Based on 2 series:
> > > > > [1] Add CMDQ secure driver for SVP
> > > > > -
> > > > > 
> > > 
> > > 
> 
> 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=785332__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj1ltVnhGY$
> > > > > 
> > > > > 
> > > > > [2] dma-buf: heaps: Add MediaTek secure heap
> > > > > -
> > > > > 
> > > 
> > > 
> 
> 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/list/?series=782776__;!!CTRNKA9wMg0ARbw!mPocbQwZ4-25DmidvAgd9K5eXjNEhSyIKpvvYHPpdrq2PgS-hkYyHohzDvoJydD45UZp5JvY9DuDVFj10sD4kHE$
> > > > > 
> > > > > ---
> > > > > 
> > > > > CK Hu (1):
> > > > >   drm/mediatek: Add interface to allocate MediaTek GEM
> > > > > buffer.
> > > > > 
> > > > > Jason-JH.Lin (9):
> > > > >   drm/mediatek/uapi: Add DRM_MTK_GEM_CREATED_ENCRYPTTED 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
> > > > >   arm64: dts: mt8195-cherry: Add secure mbox settings for
> > > > > vdosys
> > > > > 
> > > > >  .../boot/dts/mediatek/mt8195-cherry.dtsi      |  10 +
> > > > >  drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   3 +
> > > > >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c       |  31 +-
> > > > >  .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  15 +
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c       | 271
> > > > > +++++++++++++++++-
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h       |   1 +
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |  14 +
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h   |  13 +
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_drv.c        |  16 +-
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_gem.c        | 121 ++++++++
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_gem.h        |  16 ++
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_plane.c      |   7 +
> > > > >  drivers/gpu/drm/mediatek/mtk_drm_plane.h      |   2 +
> > > > >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.c       |  11 +-
> > > > >  drivers/gpu/drm/mediatek/mtk_mdp_rdma.h       |   2 +
> > > > >  include/uapi/drm/mediatek_drm.h               |  59 ++++
> > > > >  16 files changed, 575 insertions(+), 17 deletions(-)
> > > > >  create mode 100644 include/uapi/drm/mediatek_drm.h
> > > > > 

Re: [PATCH 00/10] Add mediate-drm secure flow for SVP

[Jason-JH Lin (林睿祥)]

Hi CK,

Thanks for the reviews.

[snip]

> > 
> > We'll use cmdq_sec_pkt_set_data() to bring the secure scenario,
> > secure
> > engine flags and some secure metadata to TEE world. Then will used
> > these information to make sure the pipeline is secure.
> > 
> > We don't have the secure output feature currently, such as WiFi
> > display, so we'll do it after we have to support the feature.
> > 
> > Also there are HDMITX_HDCP and DPTX_HDCP TA will check the HDCP
> > statue
> > in secure world and then CMDQ TA will get that status by calling
> > their
> > API in TEE.
> > If CMDQ TA get a HDCP checking failed sstatus, it will insert some
> > instructions in the secure cmd buffer to mute the DISP HW for each
> > frames.
> 
> You enable secure path by crtc pipeline. That means it may be primary
> display is secure and secondary display is non-secure. In
> drivers/soc/mediatek/mt8195-mmsys.h, the primary display input could
> be
> routed to secondary display output. Is mmsys protected when display
> is
> secure? The whole mmsys is protected or partial mmsys is protected? 

VDOSYS0 has 2 pipelines and considering the scenario of using VDOSYS0-0 
as secure and VDOSYS0-0 as normal, we can not protect mmsys path mux
register.

> If
> the whole mmsys is protected, the non-secure display pipeline would
> be
> malfunctioned because the control of non-secure display is in normal
> world and it may access mmsys. Please describe how this work?

For this case, we'll use CMDQ PTA to block the invalid instructions and
re-configure current mmsys path mux registers in every vblank interval
by GCE.

But we still have some works to do.
The main idea would be moving mmsys configure operation to the ATF.
Then protected the whole mmsys registers by DAPC.
So normal world can't write mmsys registers without passing smc call
cmd to ATF. ATF would check mux configure cmd is valid to current
scenario, then configures the valid mmsys mux registers.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK