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@xxxxxxxxxxxx>
> > ---
> > 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
>
