[bug report] soundwire: qcom: Check device status before reading devid
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
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
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
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
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.
participants (3)
-
Dan Carpenter -
Pierre-Louis Bossart -
Srinivas Kandagatla