Re: [bug report] soundwire: qcom: Check device status before reading devid

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 7/8/22 04:02, Srinivas Kandagatla wrote:
> 
> 
> On 08/07/2022 09:45, Dan Carpenter wrote:
>> On Fri, Jul 08, 2022 at 09:31:31AM +0100, Srinivas Kandagatla wrote:
>>> Hi Dan,
>>>
>>> On 08/07/2022 09:08, Dan Carpenter wrote:
>>>> Hello Srinivas Kandagatla,
>>>>
>>>> The patch aa1262ca6695: "soundwire: qcom: Check device status before
>>>> reading devid" from Jul 6, 2022, leads to the following Smatch static
>>>> checker warning:
>>>>
>>>>     drivers/soundwire/qcom.c:484 qcom_swrm_enumerate()
>>>>     error: buffer overflow 'ctrl->status' 11 <= 11
>>>>
>>>> drivers/soundwire/qcom.c
>>>>       471 static int qcom_swrm_enumerate(struct sdw_bus *bus)
>>>>       472 {
>>>>       473         struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
>>>>       474         struct sdw_slave *slave, *_s;
>>>>       475         struct sdw_slave_id id;
>>>>       476         u32 val1, val2;
>>>>       477         bool found;
>>>>       478         u64 addr;
>>>>       479         int i;
>>>>       480         char *buf1 = (char *)&val1, *buf2 = (char *)&val2;
>>>>       481
>>>>       482         for (i = 1; i <= SDW_MAX_DEVICES; i++) {
>>>>                        ^^^^^
>>>> This a loop that starts from 1 instead of 0.  I looked at the
>>>> surrounding context and it seems like it should be a normal loop that
>>>> starts at 0 and goes to < SDW_MAX_DEVICES.
>>>>
>>>
>>> In SoundWire world device id 0 is special one and used for
>>> enumerating the
>>> SoundWire devices.
>>>
>>> Only addresses from 1-11 are valid devids that can be assigned to
>>> devices by
>>> the host/controller.
>>>
>>> This part of the code is reading the devids assigned by the hw
>>> auto-enumeration, So the loop start from 1 is correct here.
>>>
>>>
>>>> (Or possibly the other loops are buggy as well).
>>>
>>> Atleast this code is fine, but I see other places where are starting
>>> from 0
>>> which could be fixed but the SoundWire core will ignore the status
>>> for devid
>>> 0.
>>
>> This code is *not* fine either because it should be < instead of <=.
>>
>> It might be that we always hit a zero first and break so the bug might
>> not affect users but it's still wrong.
> 
> I agree, Let me send a fix or send a v2.


the <= SDW_MAX_DEVICES is perfectly fine, provided that the allocation
is done correctly.

include/linux/soundwire/sdw.h:#define SDW_MAX_DEVICES                   11


drivers/soundwire/bus.c:        for (i = 1; i <= SDW_MAX_DEVICES; i++) {

drivers/soundwire/bus.c:        for (i = 1; i <= SDW_MAX_DEVICES; i++) {

drivers/soundwire/bus.c:        for (i = 1; i <= SDW_MAX_DEVICES; i++) {


drivers/soundwire/cadence_master.c:     for (i = 0; i <=
SDW_MAX_DEVICES; i++) {

the start at zero is intentional here.
drivers/soundwire/cadence_master.c:     enum sdw_slave_status
status[SDW_MAX_DEVICES + 1];



drivers/soundwire/qcom.c:       for (dev_num = 0; dev_num <
SDW_MAX_DEVICES; dev_num++) {


This one is a bug! device 11 is not handled

drivers/soundwire/qcom.c:       for (i = 0; i < SDW_MAX_DEVICES; i++) {


This one is a bug! device 11 is not handled

drivers/soundwire/qcom.c:       for (i = 1; i <= SDW_MAX_DEVICES; i++) {

This one has a bug! off-by-one access

drivers/soundwire/qcom.c:       enum sdw_slave_status
status[SDW_MAX_DEVICES];

Shoud be [SDW_MAX_DEVICES + 1]

The spec defines valid devices in the range 1..11 included.
Device0 is reserved for enumeration
Devices 12 and 13 are for groups
Device 14 is reserved for manager addressing
Device 15 is an 'all devices' alias.