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

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

[Zhou Qingyang]

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(-)

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:
-- 
2.25.1

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

[John Garry]

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:
>