Re: [PATCH v23 1/8] soc: cadence: Create helper functions for Cadence MHDP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Buffer overflow in cdns_mhdp_secure_mailbox_send_recv_multi() due to ignored bounds check.
- [High] Mailbox size validation bypass and permanent FIFO desynchronization for opcode 0x09.
- [Medium] Mailbox RX FIFO is not properly drained on error paths, leading to permanent desynchronization.
--

commit c962067d82ee12edcd834501898758652f04fb81
Author: Sandor Yu <Sandor.yu@nxp.com>
soc: cadence: Create helper functions for Cadence MHDP

This commit introduces mailbox communication helper functions for Cadence 
MHDP IPs. It adds support for both secure and non-secure mailbox access, 
along with register and DPCD read/write helpers.

> diff --git a/drivers/soc/cadence/cdns-mhdp-helper.c b/drivers/soc/cadence/cdns-mhdp-helper.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/soc/cadence/cdns-mhdp-helper.c
[ ... ]
> +static int mhdp_mailbox_recv_header(struct cdns_mhdp_base *base,
> +				    u8 module_id, u8 opcode,
> +				    u16 req_size, bool secure)
> +{
[ ... ]
> +	/*
> +	 * If the message in mailbox is not what we want, we need to
> +	 * clear the mailbox by reading its contents.
> +	 * Response data length for HDCP TX HDCP_TRAN_IS_REC_ID_VALID depend on
> +	 * case.
> +	 */
> +	if (opcode != header[0] ||
> +	    module_id != header[1] ||
> +	   (opcode != HDCP_TRAN_IS_REC_ID_VALID && req_size != mbox_size)) {

Does this bypass apply to any module using opcode 0x09?

Looking at the command definitions, DPTX_TRAINING_CONTROL also uses 0x09,
which means it would inadvertently bypass this size validation check since
the module_id isn't checked here.

Additionally, if this check is bypassed and the firmware actually sends an
mbox_size larger than what is dynamically read, will those unread bytes
remain in the RX FIFO? Could the next mailbox command end up reading those
stale leftover bytes as its response header, breaking synchronization?

> +		for (i = 0; i < mbox_size; i++) {
> +			if (secure)
> +				ret = mhdp_mailbox_read_secure(base);
> +			else
> +				ret = mhdp_mailbox_read(base);
> +			if (ret < 0)
> +				break;
> +		}
> +
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mhdp_mailbox_recv_data(struct cdns_mhdp_base *base,
> +				  u8 *buff, u16 buff_size, bool secure)
> +{
> +	u32 i;
> +	int ret;
> +
> +	for (i = 0; i < buff_size; i++) {
> +		if (secure)
> +			ret = mhdp_mailbox_read_secure(base);
> +		else
> +			ret = mhdp_mailbox_read(base);
> +		if (ret < 0)
> +			return ret;

If readx_poll_timeout() fails during the read, does returning directly here
leave the rest of the message stuck in the RX FIFO?

If a read times out or fails, could the next thread to acquire the
mailbox_mutex parse these leftover bytes as a new response header? Should the
FIFO be explicitly drained when an error occurs to maintain state machine
synchronization?

> +
> +		buff[i] = ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +int cdns_mhdp_secure_mailbox_send_recv_multi(struct cdns_mhdp_base *base,
> +					     u8 module_id, u8 opcode,
> +					     u16 msg_size, u8 *msg,
> +					     u8 opcode_resp,
> +					     u16 resp1_size, u8 *resp1,
> +					     u16 resp2_size, u8 *resp2)
> +{
[ ... ]
> +	ret = mhdp_mailbox_recv_data(base, resp1, resp1_size, true);
> +	if (ret) {
> +		dev_err(base->dev, "ModuleID=%d, Resp_CMD=%d recv data1 failed: %d\n",
> +			module_id, opcode_resp, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Response data length for HDCP TX HDCP_TRAN_IS_REC_ID_VALID depend on
> +	 * the number of HDCP receivers in resp1[0].
> +	 * 1 for regular case, more can be in repeater.
> +	 */
> +	if (module_id == MB_MODULE_ID_HDCP_TX &&
> +	    opcode == HDCP_TRAN_IS_REC_ID_VALID)
> +		ret = mhdp_mailbox_recv_data(base, resp2, 5 * resp1[0], true);

Could this dynamically calculated length exceed the caller-provided
resp2_size buffer capacity?

Since resp1[0] is read directly from the hardware response and could be up
to 255, this might result in writing up to 1275 bytes into the resp2
buffer. Does this need a bounds check against resp2_size to prevent memory
corruption if a misbehaving or compromised device sends a large value?

> +	else
> +		ret = mhdp_mailbox_recv_data(base, resp2, resp2_size, true);
> +	if (ret)
> +		dev_err(base->dev, "ModuleID=%d, Resp_CMD=%d recv data2 failed: %d\n",
> +			module_id, opcode_resp, ret);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-dcss-hdmi-upstreaming-v23-0-5615524a9c63@oss.nxp.com?part=1