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]

Message-ID: <c39dc157-bd3d-a627-4eb0-a34ff43ab664@linaro.org>
Date:   Wed, 7 Jun 2023 15:51:07 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     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>,
        Pierre-Louis Bossart <pierre-louis.bossart@...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 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?

Best regards,
Krzysztof