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

* [v1,1/1] drivers/usb/storage: NULL pointer dereference [null-pointer-deref] (CWE 476) problem
@ 2018-02-27 14:59 Joe Moriarty
  0 siblings, 0 replies; 10+ messages in thread
From: Joe Moriarty @ 2018-02-27 14:59 UTC (permalink / raw)
  To: Greg KH; +Cc: linux-usb

On 2/26/2018 2:35 PM, Greg KH wrote:
> On Mon, Feb 26, 2018 at 02:08:08PM -0500, Joe Moriarty wrote:
>> On 2/26/2018 1:12 PM, Greg KH wrote:
>>> 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?
>>>
>> [JDM]
>> (CWE 476) stands for Common Weakness Enumeration.
>> https://secur1ty.com/cwe/CWE-476/
>>
>> It is the type of security flaw related to a NULL pointer dereference
> 
> Ok, why put that in the subject line and not just the body of the
> changelog if you really want to call something out like this?
> 
>>>> 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?
>>
>> [JDM]
> 
> ???  You need a better email client, inline comments are the norm.
> 
>> - The messages are targeted for a developer and not the end user. I can
>> change it to dev_ calls.
> 
> But an end user sees "KERN_WARNING" messages, right?  If this is a
> debugging-only thing, then make it as such, and properly recover from it
> as it could be hit during normal operation.
> 
>>>> +
>>>>    	/*
>>>>    	 * 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?
>> [JDM]
>> - Yes, I need to return -ENXIO for the slave_alloc routine.
>>
>>>
>>>> +	}
>>>> +
>>>>    	/*
>>>>    	 * 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 :(
>> [JDM]
>> This what the static code checker is catching.  host_to_us will return NULL
>> if the following occurs.
>>
>> 'dev_to_shost()' in include/scsi/scsi_host.h line 757.
>>
>> This is done everytime a slave device is created at the following code
>> 'target_alloc()' in drivers/usb/storage/scsiglue.c  linue 340.
>>
>> This means that any call to host_to_us in the scsiglue module will have the
>> possibility of setting the return value of NULL.  In fact, I would need to
>> split out the following embedded call in 'target_alloc()' to avoid a
>> possible NULL pointer dereference.
>>
>> struct us_data *us = host_to_us(dev_to_shost(starget->dev.parent));
> 
> Exactly, your change did nothing, and us is probably not NULL here.
> 
>> The question becomes, Can a slave device ever have it's parent field set to
>> NULL (ie: dev->parent).
> 
> I don't think it can, do you see how?
> 
Ok,  I believe we went off the original problem this patch solves. 
Since you and I both agree that a slave device can never have it's 
parent field set to NULL (ie:  dev->parent) then this patch boils down 
to the following one change.

include/scsi/scsi_host.h
static inline struct Scsi_Host *dev_to_shost(struct device *dev)
{
	while (!scsi_is_host_device(dev)) {
		if (!dev->parent)
-			return NULL;
+			BUG();
		dev = dev->parent;
	}
	return container_of(dev, struct Scsi_Host, shost_gendev);
}

If you agree, then I can revert all the other changes and resubmit a 
version 2 of this patch with this change.  If you do not agree, then I 
will mark it as a false positive and move on, or we can continue to work 
on the initial patch to get it to your satisfaction.  Let me know how 
you would like to proceed here.

Joe

>> Can it?  If not, this becomes a false positive that needs to be
>> ignored and the entire patch is not needed.  I would tend to believe a
>> slave device will always be enumerated by the parent (host) device.
>> Correct?
> 
> How else can it be created?
> 
>>>> +	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?
>>
>> [JDM] I need to return -ENXIO here.
> 
> -ENODEV?
> 
> But again, how can this happen?
> 
> thanks,
> 
> 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
>
---
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

^ permalink raw reply	[flat|nested] 10+ messages in thread