| Message ID | 20211130171629.201026-1-zhou1615@umn.edu |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: libsas: Fix a NULL pointer dereference in sas_ex_discover_expander() | expand |
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@umn.edu> > --- > 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: >
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)) { + sas_put_device(child); + return NULL; + } + if (sas_port_add(phy->port) != 0) { + sas_port_free(phy->port); + sas_put_device(child); + return NULL; + } switch (phy->attached_dev_type) { case SAS_EDGE_EXPANDER_DEVICE:
In sas_ex_discover_expander(), sas_port_alloc() is assigned to phy->port 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(). 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. Builds with CONFIG_SCSI_SAS_LIBSAS=m show no new warnings, and our static analyzer no longer warns about this code. Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") Signed-off-by: Zhou Qingyang <zhou1615@umn.edu> --- drivers/scsi/libsas/sas_expander.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)