Re: [PATCH v2 09/11] drm/mediatek: Add secure flow support to mediatek-drm

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

Hi CK,

Thanks for the reviews.

On Tue, 2023-10-24 at 07:42 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-10-23 at 12:45 +0800, Jason-JH.Lin wrote:
> > To add secure flow support for mediatek-drm, each crtc have to
> > create a secure cmdq mailbox channel. Then cmdq packets with
> > display HW configuration will be sent to secure cmdq mailbox
> > channel
> > and configured in the secure world.
> > 
> > Each crtc have to use secure cmdq interface to configure some
> > secure
> > settings for display HW before sending cmdq packets to secure cmdq
> > mailbox channel.
> > 
> > If any of fb get from current drm_atomic_state is secure, then crtc
> > will switch to the secure flow to configure display HW.
> > If all fbs are not secure in current drm_atomic_state, then crtc
> > will
> > switch to the normal flow.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 272
> > ++++++++++++++++++++++-
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.h  |   1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c |   7 +
> >  3 files changed, 269 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index b6fa4ad2f94d..6c2cf339b923 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -56,6 +56,11 @@ struct mtk_drm_crtc {
> >  	u32				cmdq_event;
> >  	u32				cmdq_vblank_cnt;
> >  	wait_queue_head_t		cb_blocking_queue;
> > +
> > +	struct cmdq_client		sec_cmdq_client;
> > +	struct cmdq_pkt			sec_cmdq_handle;
> > +	bool				sec_cmdq_working;
> > +	wait_queue_head_t		sec_cb_blocking_queue;
> >  #endif
> >  
> >  	struct device			*mmsys_dev;
> > @@ -67,6 +72,7 @@ struct mtk_drm_crtc {
> >  	/* lock for display hardware access */
> >  	struct mutex			hw_lock;
> >  	bool				config_updating;
> > +	bool				sec_on;
> >  };
> >  
> >  struct mtk_crtc_state {
> > @@ -109,6 +115,154 @@ static void mtk_drm_finish_page_flip(struct
> > mtk_drm_crtc *mtk_crtc)
> >  	}
> >  }
> >  
> > +void mtk_crtc_disable_secure_state(struct drm_crtc *crtc)
> > +{
> > +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > +	enum cmdq_sec_scenario sec_scn = CMDQ_MAX_SEC_COUNT;
> > +	int i;
> > +	struct mtk_ddp_comp *ddp_first_comp;
> > +	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +	u64 sec_engine = 0; /* for hw engine write output secure fb */
> > +	u64 sec_port = 0; /* for larb port read input secure fb */
> > +
> > +	mutex_lock(&mtk_crtc->hw_lock);
> > +
> > +	if (!mtk_crtc->sec_cmdq_client.chan) {
> > +		pr_err("crtc-%d secure mbox channel is NULL\n",
> > drm_crtc_index(crtc));
> > +		goto err;
> > +	}
> > +
> > +	if (!mtk_crtc->sec_on) {
> > +		pr_debug("crtc-%d is already disabled!\n",
> > drm_crtc_index(crtc));
> > +		goto err;
> > +	}
> > +
> > +	mbox_flush(mtk_crtc->sec_cmdq_client.chan, 0);
> > +	mtk_crtc->sec_cmdq_handle.cmd_buf_size = 0;
> > +
> > +	if (mtk_crtc->sec_cmdq_handle.sec_data) {
> > +		struct cmdq_sec_data *sec_data;
> > +
> > +		sec_data = mtk_crtc->sec_cmdq_handle.sec_data;
> > +		sec_data->addrMetadataCount = 0;
> > +		sec_data->addrMetadatas = (uintptr_t)NULL;
> > +	}
> > +
> > +	/*
> > +	 * Secure path only support DL mode, so we just wait
> > +	 * the first path frame done here
> > +	 */
> > +	cmdq_pkt_wfe(&mtk_crtc->sec_cmdq_handle, mtk_crtc->cmdq_event,
> > false);
> > +
> > +	ddp_first_comp = mtk_crtc->ddp_comp[0];
> > +	for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > +		struct drm_plane *plane = &mtk_crtc->planes[i];
> > +
> > +		sec_port |=
> > mtk_ddp_comp_layer_get_sec_port(ddp_first_comp, i);
> > +
> > +		/* make sure secure layer off before switching secure
> > state */
> > +		if (!mtk_plane_fb_is_secure(plane->state->fb)) {
> > +			struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> > +
> > +			plane_state->pending.enable = false;
> > +			mtk_ddp_comp_layer_config(ddp_first_comp, i,
> > plane_state,
> > +						  &mtk_crtc-
> > > sec_cmdq_handle);
> 
> You disable layer here and disable secure path in
> cmdq_sec_pkt_set_data() later. But this is real world and could be
> hacked by hacker. If hacker do not disable layer here but disable
> secure path in cmdq_sec_pkt_set_data() later, the hardware would keep
> reading secure buffer and path is not secure? That means video could
> output to HDMI without HDCP?

Disabling secure path by cmdq_sec_pkt_set_data() will also switch the
larb used by OVL to non-secure identity. So even if the secure layer is
enabled, OVL can not access secure DRAM with non-secure larb.

And it will cause a IOMMU translation fault when non-secure larb access
the address of secure DRAM.

Regards,
Jason-JH.Lin

> 
> Regards,
> CK 
>