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

[Srinivas Kandagatla]

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.

--srini
> 
>      483                 /* do not continue if the status is Not Present  */
> --> 484                 if (!ctrl->status[i])
> 
> So this is off by one and reads one element beyond the end of the loop.
> 
>      485                         continue;
>      486
>      487                 /*SCP_Devid5 - Devid 4*/
>      488                 ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_1(i), &val1);
>      489
>      490                 /*SCP_Devid3 - DevId 2 Devid 1 Devid 0*/
>      491                 ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_2(i), &val2);
>      492
>      493                 if (!val1 && !val2)
>      494                         break;
>      495
>      496                 addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>      497                         ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>      498                         ((u64)buf1[0] << 40);
>      499
>      500                 sdw_extract_slave_id(bus, addr, &id);
>      501                 found = false;
>      502                 /* Now compare with entries */
>      503                 list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>      504                         if (sdw_compare_devid(slave, id) == 0) {
>      505                                 qcom_swrm_set_slave_dev_num(bus, slave, i);
>      506                                 found = true;
>      507                                 break;
>      508                         }
>      509                 }
>      510
>      511                 if (!found) {
>      512                         qcom_swrm_set_slave_dev_num(bus, NULL, i);
>      513                         sdw_slave_add(bus, &id, NULL);
>      514                 }
>      515         }
>      516
>      517         complete(&ctrl->enumeration);
>      518         return 0;
>      519 }
> 
> regards,
> dan carpenter