Re: [PATCH 07/21] soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] soundwire: qcom: port_mask leak on error in qcom_swrm_stream_alloc_ports()
--

commit 67cffb71aa319b54456dfa761fc2fc2da4b71496
Author: Lee Jones <lee@kernel.org>

soundwire: qcom: Prevent pconfig buffer overflow in qcom_swrm_stream_alloc_ports()

This commit adds a bounds check in qcom_swrm_stream_alloc_ports() to verify
that nports is less than maxport before writing to pconfig[nports], preventing
an out-of-bounds write when more slave ports are requested than ctrl->nports.

> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index 6e0c5fb10efad..2050112340302 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -1343,6 +1343,11 @@ static int qcom_swrm_stream_alloc_ports(struct qcom_swrm_ctrl *ctrl,
>  		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
>  			slave = s_rt->slave;
>  			list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> +				if (nports >= maxport) {
> +					dev_err(ctrl->dev, "All ports busy\n");
> +					return -EBUSY;
> +				}

[Severity: High]
Does returning -EBUSY here leak the port allocations in ctrl->port_mask?

In qcom_swrm_stream_alloc_ports(), each loop iteration over s_rt->port_list
sets bit pn in ctrl->port_mask and records pn in the local pconfig array:

	set_bit(pn, port_mask);
	pconfig[nports].num = pn;
	pconfig[nports].ch_mask = p_rt->ch_mask;
	nports++;

When nports reaches maxport on a subsequent iteration and returns -EBUSY (or
when the pn >= maxport check below triggers after earlier iterations already
succeeded), qcom_swrm_stream_alloc_ports() returns before calling
sdw_stream_add_master().

Although the caller qcom_swrm_hw_params() invokes
qcom_swrm_stream_free_ports() on error:

	ret = qcom_swrm_stream_alloc_ports(ctrl, sruntime, params,
					   substream->stream);
	if (ret)
		qcom_swrm_stream_free_ports(ctrl, sruntime);

qcom_swrm_stream_free_ports() only clears bits in ctrl->port_mask by iterating
over m_rt->port_list:

	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
		port_mask = &ctrl->port_mask;
		list_for_each_entry(p_rt, &m_rt->port_list, port_node)
			clear_bit(p_rt->num, port_mask);
	}

Because m_rt->port_list is only populated inside sdw_stream_add_master() and
pconfig is freed on return, m_rt->port_list is still empty when
qcom_swrm_stream_alloc_ports() fails, leaving all previously set bits in
ctrl->port_mask permanently set.

Should qcom_swrm_stream_alloc_ports() unwind the bits set in port_mask from
pconfig[0 .. nports - 1] before returning -EBUSY?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030104840.3361217-1-lee@kernel.org?part=7