Re: [PATCH] soundwire: qcom: fix storing port config out-of-bounds

[Pierre-Louis Bossart]

On 6/7/23 08:51, Krzysztof Kozlowski wrote:
> On 07/06/2023 15:10, Vinod Koul wrote:
>> On 01-06-23, 12:25, Krzysztof Kozlowski wrote:
>>> The 'qcom_swrm_ctrl->pconfig' has size of QCOM_SDW_MAX_PORTS (14),
>>> however we index it starting from 1, not 0, to match real port numbers.
>>> This can lead to writing port config past 'pconfig' bounds and
>>> overwriting next member of 'qcom_swrm_ctrl' struct.  Reported also by
>>> smatch:
>>>
>>>   drivers/soundwire/qcom.c:1269 qcom_swrm_get_port_config() error: buffer overflow 'ctrl->pconfig' 14 <= 14
>>>
>>> Fixes: 9916c02ccd74 ("soundwire: qcom: cleanup internal port config indexing")
>>> Cc: <stable@xxxxxxxxxxxxxxx>
>>> Reported-by: kernel test robot <lkp@xxxxxxxxx>
>>> Reported-by: Dan Carpenter <error27@xxxxxxxxx>
>>> Link: https://lore.kernel.org/r/202305201301.sCJ8UDKV-lkp@xxxxxxxxx/
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>>> ---
>>>  drivers/soundwire/qcom.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>>> index 7cb1b7eba814..88a772075907 100644
>>> --- a/drivers/soundwire/qcom.c
>>> +++ b/drivers/soundwire/qcom.c
>>> @@ -202,7 +202,8 @@ struct qcom_swrm_ctrl {
>>>  	u32 intr_mask;
>>>  	u8 rcmd_id;
>>>  	u8 wcmd_id;
>>> -	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
>>> +	/* Port numbers are 1 - 14 */
>>> +	struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS + 1];
>>
>> Better use SDW_MAX_PORTS ?
> 
> That's interesting idea, but except of value, is the meaning actually
> the same? Driver claims that port 0 is masked and max number of ports is
> 14. Therefore it uses in all places constant QCOM_SDW_MAX_PORTS. We need
> here +1, only because we index from 1, not 0, but we still index over
> QCOM_SDW_MAX_PORTS, not SDW_MAX_PORTS. Wouldn't it be also confusing to
> use here SDW_MAX_PORTS but then index over something else?

SDW_MAX_PORTS only applies for the peripheral. DP0 is reserved for
non-audio/Bulk request, DP15 is an alias for "all ports"

There's nothing in the spec that restricts the ports on the manager
side, be it to dedicate Port0 or Port15 to a specific purpose or even
the number of ports.

I would recommend using a vendor-specific definition rather than
overloading a peripheral specification requirement.