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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 7 Jun 2023 09:51:44 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Vinod Koul <vkoul@...nel.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        linux-arm-msm@...r.kernel.org, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        kernel test robot <lkp@...el.com>,
        Dan Carpenter <error27@...il.com>
Subject: Re: [PATCH] soundwire: qcom: fix storing port config out-of-bounds



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@...r.kernel.org>
>>> Reported-by: kernel test robot <lkp@...el.com>
>>> Reported-by: Dan Carpenter <error27@...il.com>
>>> Link: https://lore.kernel.org/r/202305201301.sCJ8UDKV-lkp@intel.com/
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
>>> ---
>>>  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.