On 5/24/24 12:23, Dan Carpenter wrote:
> Hello Tomas Henzl,
Hi Dan,
sorry for the delay, I was offline for a longer time.
Thanks for pointing out the issues.
>
> Commit 3668651def2c ("scsi: mpi3mr: Sanitise num_phys") from Feb 26,
> 2024 (linux-next), leads to the following Smatch static checker
> warning:
>
> drivers/scsi/mpi3mr/mpi3mr_transport.c:1371 mpi3mr_sas_port_add()
> warn: array off by one? 'mr_sas_node->phy[i]'
>
> drivers/scsi/mpi3mr/mpi3mr_transport.c
> 1352 mr_sas_port->hba_port = hba_port;
> 1353 mpi3mr_sas_port_sanity_check(mrioc, mr_sas_node,
> 1354 mr_sas_port->remote_identify.sas_address, hba_port);
> 1355
> 1356 if (mr_sas_node->num_phys > sizeof(mr_sas_port->phy_mask) * 8)
> 1357 ioc_info(mrioc, "max port count %u could be too high\n",
> 1358 mr_sas_node->num_phys);
> 1359
> 1360 for (i = 0; i < mr_sas_node->num_phys; i++) {
> 1361 if ((mr_sas_node->phy[i].remote_identify.sas_address !=
> 1362 mr_sas_port->remote_identify.sas_address) ||
> 1363 (mr_sas_node->phy[i].hba_port != hba_port))
> 1364 continue;
> 1365
> 1366 if (i > sizeof(mr_sas_port->phy_mask) * 8) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This check is wrong. It should be >=. But also ->phy_mask is a u64
> when probably it should be a u32.
The test should be corrected I'm about to sent a patch.
Cheers,
Tomas
>
> 1367 ioc_warn(mrioc, "skipping port %u, max allowed value is %zu\n",
> 1368 i, sizeof(mr_sas_port->phy_mask) * 8);
> 1369 goto out_fail;
> 1370 }
> --> 1371 list_add_tail(&mr_sas_node->phy[i].port_siblings,
> 1372 &mr_sas_port->phy_list);
> 1373 mr_sas_port->num_phys++;
> 1374 mr_sas_port->phy_mask |= (1 << i);
> ^^^^^^
> There are a bunch of "1 << i" shifts in this file and they'll shift wrap
> if i >= 32. Then the ->phy_mask is tested with ffs() which takes an
> int. So everything above bit 31 is not going to work.
>
> 1375 }
> 1376
> 1377 if (!mr_sas_port->num_phys) {
> 1378 ioc_err(mrioc, "failure at %s:%d/%s()!\n",
> 1379 __FILE__, __LINE__, __func__);
> 1380 goto out_fail;
> 1381 }
>
> regards,
> dan carpenter
>
