Re: [PATCH] scsi: libsas: Fix a NULL pointer dereference in sas_ex_discover_expander()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux