Re: [PATCH v2 8/9] mailbox: mediatek: Add CMDQ secure mailbox driver

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

Hi CK,

On Mon, 2023-11-06 at 06:53 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
> 
> On Mon, 2023-10-23 at 12:37 +0800, Jason-JH.Lin wrote:
> > To support secure video path feature, GCE have to read/write
> > registgers
> > in the secure world. GCE will enable the secure access permission
> > to
> > the
> > HW who wants to access the secure content buffer.
> > 
> > Add CMDQ secure mailbox driver to make CMDQ client user is able to
> > sending their HW settings to the secure world. So that GCE can
> > execute
> > all instructions to configure HW in the secure world.
> > 
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> >  drivers/mailbox/Makefile                      |    2 +-
> >  drivers/mailbox/mtk-cmdq-sec-mailbox.c        | 1102
> > +++++++++++++++++
> >  drivers/mailbox/mtk-cmdq-sec-tee.c            |  202 +++
> >  include/linux/mailbox/mtk-cmdq-mailbox.h      |    2 +
> >  .../linux/mailbox/mtk-cmdq-sec-iwc-common.h   |  293 +++++
> >  include/linux/mailbox/mtk-cmdq-sec-mailbox.h  |   83 ++
> >  include/linux/mailbox/mtk-cmdq-sec-tee.h      |   31 +
> >  7 files changed, 1714 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mailbox/mtk-cmdq-sec-mailbox.c
> >  create mode 100644 drivers/mailbox/mtk-cmdq-sec-tee.c
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-iwc-common.h
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-mailbox.h
> >  create mode 100644 include/linux/mailbox/mtk-cmdq-sec-tee.h
> > 
> > 
> 
> [snip]
> 
> > +
> > +int cmdq_sec_insert_backup_cookie(struct cmdq_pkt *pkt)
> > +{
> > +	struct cmdq_client *cl = (struct cmdq_client *)pkt->cl;
> > +	struct cmdq_sec_thread *thread = ((struct mbox_chan *)(cl-
> > > chan))->con_priv;
> > 
> > +	struct cmdq_sec *cmdq = container_of(thread->chan->mbox, struct
> > cmdq_sec, mbox);
> > +	struct cmdq_operand left, right;
> > +	dma_addr_t addr;
> > +
> > +	if (!thread->occupied || !cmdq->shared_mem)
> > +		return -EFAULT;
> > +
> > +	pr_debug("%s %d: pkt:%p thread:%u gce:%#lx",
> > +		 __func__, __LINE__, pkt, thread->idx, (unsigned
> > long)cmdq->base_pa);
> > +
> > +	addr = (u32)(cmdq->base_pa + CMDQ_THR_BASE +
> > +		CMDQ_THR_SIZE * thread->idx + CMDQ_THR_EXEC_CNT_PA);
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> > +
> > +	left.reg = true;
> > +	left.idx = CMDQ_THR_SPR_IDX1;
> > +	right.reg = false;
> > +	right.value = 1;
> > +	cmdq_pkt_logic_command(pkt, CMDQ_LOGIC_ADD, CMDQ_THR_SPR_IDX1,
> > &left, &right);
> > +
> > +	addr = cmdq->shared_mem->pa + CMDQ_SEC_SHARED_THR_CNT_OFFSET +
> > +		thread->idx * sizeof(u32);
> > +
> > +	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_HIGH(addr));
> > +	cmdq_pkt_write_s(pkt, CMDQ_THR_SPR_IDX2, CMDQ_ADDR_LOW(addr),
> > CMDQ_THR_SPR_IDX1);
> 
> This is a sample code of reading secure data and writing to normal
> world. I think you have already check address of cmdq_pkt_read_s() in
> TEE. This is a sample code that hacker may try to read any secure
> address:
> 
> 	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1,
> CMDQ_ADDR_HIGH(hack_address));
> 	cmdq_pkt_jump(pkt, read);
> 	cmdq_pkt_assign(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_HIGH(addr));
> read:
> 	cmdq_pkt_read_s(pkt, CMDQ_THR_SPR_IDX1, CMDQ_ADDR_LOW(addr),
> 		CMDQ_THR_SPR_IDX1);
> 
> Please explain how you protect this hacker attack.
> 

Yes, hackers can use this sample code to read the HW register to the
shared memory, but they still can't get the secure content or other
useful information to get the secure content.

There are some protections we made for this action:
1. All the physical address of HW register in the read/write
instruction in secure command buffer need to be defined at whitelist.
2. Reading DRAM address instruction in secure command buffer are
forbidden.
3. The shared memory is used to store the value of cookie which is used
for secure cmdq packet scheduling. So secure GCE thread executing state
will crash by modifying it unexpectedly.

Although hackers could get the value store in the HW register by this
sample code, they still don't know what the HW register stand for and 
what is the meaning of the value they read. Additionally, if they read
unexpectedly values into shared memory, they will corrupt the secure
GCE thread state.

Regards,
Jason-JH.Lin

> Regards,
> CK
> 
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> > +