[v1,1/1] drivers/usb/storage: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Feb 26, 2018 at 12:10:02PM -0500, Joe Moriarty wrote:
> The Parfait (version 2.1.0) static code analysis tool found the
> following NULL pointer dereference problem.

What is the "CWE 476" thing in the subject line for?

> 
> dev_to_shost() in include/scsi/scsi_host.h has the ability to return
> NULL if the scsi host device does not have the Scsi_host->parent
> field set.  With the possibilty of a NULL pointer being set for
> the Scsi_Host->parent field, calls to host_to_us() have to make
> sure the return pointer is not null.  Changes were made to check
> for a return value of NULL on calls to host_to_us().
> 
> Signed-off-by: Joe Moriarty <joe.moriarty@oracle.com>
> Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
> Acked-by: Hakon Bugge <hakon.bugge@oracle.com>
> ---
>  drivers/usb/storage/scsiglue.c | 53 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
> index c267f2812a04..94af609d49bf 100644
> --- a/drivers/usb/storage/scsiglue.c
> +++ b/drivers/usb/storage/scsiglue.c
> @@ -66,6 +66,9 @@ static int slave_alloc (struct scsi_device *sdev)
>  {
>  	struct us_data *us = host_to_us(sdev->host);
>  
> +	if (!us)
> +		pr_warn("Error in %s: us = NULL\n", __func__);

It is a driver, you should never use pr_* calls, but rather dev_* calls.

Also, you don't exit, are you sure the code keeps working after this
happens?

And what is a user supposed to do with this warning message?

> +
>  	/*
>  	 * Set the INQUIRY transfer length to 36.  We don't use any of
>  	 * the extra data and many devices choke if asked for more or
> @@ -102,6 +105,11 @@ static int slave_configure(struct scsi_device *sdev)
>  {
>  	struct us_data *us = host_to_us(sdev->host);
>  
> +	if (!us) {
> +		pr_warn("Error in %s: us = NULL\n", __func__);
> +		return 0;

Why are you returning a success?

> +	}
> +
>  	/*
>  	 * Many devices have trouble transferring more than 32KB at a time,
>  	 * while others have trouble with more than 64K. At this time we
> @@ -331,6 +339,11 @@ static int target_alloc(struct scsi_target *starget)
>  {
>  	struct us_data *us = host_to_us(dev_to_shost(starget->dev.parent));

Can host_to_us() handle NULL?

Nope, just looked at it, this will never cause the return value to be
NULL, your checker needs some more work :(

>  
> +	if (!us) {
> +		pr_warn("Error in %s: us = NULL\n", __func__);
> +		return 0;
> +	}
> +
>  	/*
>  	 * Some USB drives don't support REPORT LUNS, even though they
>  	 * report a SCSI revision level above 2.  Tell the SCSI layer
> @@ -361,6 +374,11 @@ static int queuecommand_lck(struct scsi_cmnd *srb,
>  {
>  	struct us_data *us = host_to_us(srb->device->host);
>  
> +	if (!us) {

How is that possible?  This doesn't have anything to do with your
subject line, right?

> +		pr_warn("Error in %s: us = NULL\n", __func__);
> +		return 0;

success?

I stopped reviewing here, sorry :(

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html