* [bug report] soundwire: qcom: Check device status before reading devid
@ 2022-07-08 8:08 Dan Carpenter
2022-07-08 8:31 ` Srinivas Kandagatla
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-07-08 8:08 UTC (permalink / raw)
To: srinivas.kandagatla; +Cc: alsa-devel
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.
(Or possibly the other loops are buggy as well).
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] soundwire: qcom: Check device status before reading devid
2022-07-08 8:08 [bug report] soundwire: qcom: Check device status before reading devid Dan Carpenter
@ 2022-07-08 8:31 ` Srinivas Kandagatla
2022-07-08 8:45 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Srinivas Kandagatla @ 2022-07-08 8:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] soundwire: qcom: Check device status before reading devid
2022-07-08 8:31 ` Srinivas Kandagatla
@ 2022-07-08 8:45 ` Dan Carpenter
2022-07-08 9:02 ` Srinivas Kandagatla
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-07-08 8:45 UTC (permalink / raw)
To: Srinivas Kandagatla; +Cc: alsa-devel
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.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] soundwire: qcom: Check device status before reading devid
2022-07-08 8:45 ` Dan Carpenter
@ 2022-07-08 9:02 ` Srinivas Kandagatla
2022-07-08 14:04 ` Pierre-Louis Bossart
0 siblings, 1 reply; 5+ messages in thread
From: Srinivas Kandagatla @ 2022-07-08 9:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: alsa-devel
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.
--srini
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] soundwire: qcom: Check device status before reading devid
2022-07-08 9:02 ` Srinivas Kandagatla
@ 2022-07-08 14:04 ` Pierre-Louis Bossart
0 siblings, 0 replies; 5+ messages in thread
From: Pierre-Louis Bossart @ 2022-07-08 14:04 UTC (permalink / raw)
To: Srinivas Kandagatla, Dan Carpenter; +Cc: alsa-devel, Vinod Koul
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-08 14:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-08 8:08 [bug report] soundwire: qcom: Check device status before reading devid Dan Carpenter
2022-07-08 8:31 ` Srinivas Kandagatla
2022-07-08 8:45 ` Dan Carpenter
2022-07-08 9:02 ` Srinivas Kandagatla
2022-07-08 14:04 ` Pierre-Louis Bossart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.