Re: [PATCH RFC v7 3/9] firmware: arm_scmi: vendors: Add QCOM SCMI Generic Extensions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing bounds check on firmware response length causes potential stack buffer overflow or uninitialized memory read.
- [Medium] Undefined behavior in `memcpy` when passed a NULL buffer for a zero-length payload.
--

commit 66dbf88ec6a6058435ff2cd14122285e5de5a740
Author: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

firmware: arm_scmi: vendors: Add QCOM SCMI Generic Extensions

This commit introduces the QCOM SCMI Generic Extensions Protocol, providing
a generic way to expose Qualcomm SoC specific features via the SCMI controller.

> diff --git a/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> new file mode 100644
> index 0000000000000..77c07272c09d6
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c

[ ... ]

> +static int qcom_scmi_common_xfer(const struct scmi_protocol_handle *ph,
> +				 enum qcom_generic_ext_protocol_cmd cmd_id, void *buf,
> +				 size_t buf_len, u64 algo_str, u32 param_id, size_t rx_size)
> +{
> +	struct scmi_xfer *t;
> +	struct qcom_scmi_msg *msg;
> +	int ret;
> +
> +	/* Reject calls where rx_size exceeds buf_len. */
> +	if (rx_size > buf_len)
> +		return -EINVAL;
> +
> +	ret = ph->xops->xfer_get_init(ph, cmd_id, buf_len + sizeof(*msg), rx_size, &t);
> +	if (ret)
> +		return ret;
> +
> +	msg = t->tx.buf;
> +	msg->ext_id = 0;
> +	msg->algo_low = cpu_to_le32(lower_32_bits(algo_str));
> +	msg->algo_high = cpu_to_le32(upper_32_bits(algo_str));
> +	msg->param_id = cpu_to_le32(param_id);
> +	memcpy(msg->buf, buf, buf_len);

[Severity: Medium]
Is it possible for this memcpy() to receive a NULL buffer?

If a caller invokes a start or stop activity command with a zero-length payload
(passing NULL for buf and 0 for buf_len), this will result in memcpy() being
called with a NULL source pointer.

Calling memcpy() with a NULL pointer is undefined behavior in standard C,
even if the length is zero, and can trigger warnings when Fortify Source
or UBSAN are enabled.

Should this copy be guarded by a check for buf_len > 0?

> +
> +	ret = ph->xops->do_xfer(ph, t);
> +	if (!ret && rx_size)
> +		/*
> +		 * Response is returned into the caller's @buf, replacing the
> +		 * tx payload. Callers using the same on-stack struct for both
> +		 * directions must not rely on tx contents after this point.
> +		 */
> +		memcpy(buf, t->rx.buf, t->rx.len);

[Severity: Critical]
Does this memcpy() need bounds checking on the received length?

The length t->rx.len is controlled by firmware. If the firmware returns a
payload larger than the caller's allocated buffer (buf_len), this copy could
overflow the caller's stack or buffer.

Conversely, if the firmware returns a truncated response (t->rx.len < rx_size),
fewer bytes will be copied. Since the function returns 0 on success without
reporting the actual received length, the caller might unknowingly read leftover
TX payload as valid RX data.

Would it be safer to validate t->rx.len against buf_len or rx_size, and return
an error if the length does not match expectations?

> +	ph->xops->xfer_put(ph, t);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-rfc_v7_scmi_memlat-v7-0-f3f68c608f25@oss.qualcomm.com?part=3