Re: [PATCH RFC 03/10] firmware: arm_scmi: vendors: Add QCOM SCMI Generic Extensions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The API tightly couples TX payload size and RX buffer capacity into a single `buf_len` parameter, breaking commands where RX size exceeds TX size and leaking uninitialized kernel memory.
- [High] An integer overflow in the transmit size calculation bypasses SCMI core bounds checks, leading to an out-of-bounds write.
- [Medium] The 64-bit constant representing the "MEMLAT" algorithm string is incorrectly split and endian-swapped, resulting in the mangled string "TALMEM\0\0" being laid out in memory.
--

--- Patch [3]: [PATCH RFC 03/10] firmware: arm_scmi: vendors: Add QCOM SCMI Generic Extensions ---
commit 9dcfeae2ae27c1a2917b7de276eae7604cac163d
Author: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

firmware: arm_scmi: vendors: Add QCOM SCMI Generic Extensions
    
The System Control Management Interface (SCMI) Qualcomm Generic Extension
Protocol consists of a small set of generic SET/GET/ START/STOP commands,
which is used to turn on/off and configure Qualcomm SoC specific algorithms
that run on the SCP.

> 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..7ca920c8da059
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/vendors/qcom/qcom-generic-ext.c
> @@ -0,0 +1,183 @@
[ ... ]
> +/*
> + * The firmware only implements a handful of algorithm strings. Keep an
> + * allowlist so a client cannot push an unsupported (or garbage) string to
> + * the firmware; extend it as new algorithms are added.
> + */
> +static const u64 qcom_scmi_algo_str[] = {
> +	0x4d454d4c4154ULL, /* "MEMLAT" */
> +};
[ ... ]
> +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;
> +
> +	if (!qcom_scmi_algo_str_valid(algo_str))
> +		return -EINVAL;
> +
> +	/* 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);

[Severity: High]
Can buf_len + sizeof(*msg) overflow?

If a caller passes a very large buf_len, could the addition wrap around to a
small positive integer? 

This would bypass the max_msg_size bounds check in xfer_get_init(), leading
to an out-of-bounds write in the subsequent memcpy() call.

> +	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));

[Severity: Medium]
Does the firmware expect this string as an 8-byte character array?

The constant 0x4d454d4c4154ULL represents "MEMLAT" in big-endian hex. When
this is split into algo_low and algo_high and passed through cpu_to_le32(),
the resulting little-endian memory layout becomes 0x54 0x41 0x4C 0x4D ("TALM")
followed by 0x45 0x4D 0x00 0x00 ("EM\0\0").

Will the firmware correctly reconstruct the 64-bit integer, or might it
reject the mangled string "TALMEM\0\0" if it reads it as a character array?

> +	msg->param_id = cpu_to_le32(param_id);
> +	if (buf_len)
> +		memcpy(msg->buf, buf, buf_len);

[Severity: High]
If a caller uses qcom_scmi_get_param() expecting an RX payload but has no TX
payload, doesn't buf_len need to be at least rx_size to pass the validation
check at the top of this function?

If buf_len is artificially inflated to satisfy rx_size <= buf_len, won't this
memcpy() read buf_len bytes from the caller's buffer into the TX payload,
potentially leaking uninitialized kernel memory to the firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-rfc_v8_scmi_memlat-v1-0-cb732bcff1f4@oss.qualcomm.com?part=3