On 30/11/2021 17:16, Zhou Qingyang wrote:
I'd have "scsi: libsas: Improve error handling in
sas_ex_discover_expander()"
In sas_ex_discover_expander(), sas_port_alloc() is assigned to phy->port
"sas_port_alloc() is assigned to phy->port" - the function is not assigned
and used in sas_port_add(). sas_port_add() further passes phy->port to
list_empty(), and there is a dereference of it in list_empty(), which
could lead to a NULL pointer dereference on failure of
sas_port_alloc().
This patch imitates the same error-handling logic in
sas_ex_discover_end_dev().
git grep 'This patch' Documentation/process/submitting-patches.rst
Fix this bug by adding checks for phy->port and sas_port_add().
This bug was found by a static analyzer. The analysis employs
differential checking to identify inconsistent security operations
(e.g., checks or kfrees) between two code paths and confirms that the
inconsistent operations are not recovered in the current function or
the callers, so they constitute bugs.
Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger. Multiple researchers have cross-reviewed
the bug.
Who are these researchers?
Builds with CONFIG_SCSI_SAS_LIBSAS=m show no new warnings,
and our static analyzer no longer warns about this code.
This is all implied by sending the patch in the first place
Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver")
personally I don't think that this is a fix - the code is old and
already had BUG_ON()
Signed-off-by: Zhou Qingyang <zhou1615@xxxxxxx>
---
drivers/scsi/libsas/sas_expander.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index c2150a818423..7530b1773d6b 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -957,9 +957,16 @@ static struct domain_device *sas_ex_discover_expander(
return NULL;
phy->port = sas_port_alloc(&parent->rphy->dev, phy_id);
- /* FIXME: better error handling */
- BUG_ON(sas_port_add(phy->port) != 0);
+ if (unlikely(!phy->port)) {
no need for unlikely() - this is not fastpath
+ sas_put_device(child);
+ return NULL;
+ }
+ if (sas_port_add(phy->port) != 0) {
+ sas_port_free(phy->port);
+ sas_put_device(child);
better have a goto error now as we're replicting code, including what is
already there for the sas_discover_expander() failure error path
+ return NULL;
+ }
switch (phy->attached_dev_type) {
case SAS_EDGE_EXPANDER_DEVICE:
