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

[CK Hu (胡俊光)]

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

Regards,
CK

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(cmdq_sec_insert_backup_cookie);
> +