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

CK Hu (胡俊光)
2023-11-06T06:53:47Z

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.

Regards,
CK

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