lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Wed, 25 Oct 2023 08:31:20 +0000
From:   Jason-JH Lin (林睿祥) 
        <Jason-JH.Lin@...iatek.com>
To:     CK Hu (胡俊光) <ck.hu@...iatek.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Singo Chang (張興國) 
        <Singo.Chang@...iatek.com>,
        Johnson Wang (王聖鑫) 
        <Johnson.Wang@...iatek.com>,
        "linaro-mm-sig@...ts.linaro.org" <linaro-mm-sig@...ts.linaro.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        Jason-ch Chen (陳建豪) 
        <Jason-ch.Chen@...iatek.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Shawn Sung (宋孝謙) 
        <Shawn.Sung@...iatek.com>,
        Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
        "jkardatzke@...gle.com" <jkardatzke@...gle.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 09/11] drm/mediatek: Add secure flow support to
 mediatek-drm

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@...iatek.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 
> 

Powered by blists - more mailing lists