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
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@xxxxxxxxxx>
Reviewed-by: Steven Sistare <steven.sistare@xxxxxxxxxx>
Acked-by: Hakon Bugge <hakon.bugge@xxxxxxxxxx>
---
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]
- The messages are targeted for a developer and not the end user. I can
change it to dev_ calls.
+
/*
* 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));
The question becomes, Can a slave device ever have it's parent field set
to NULL (ie: dev->parent). 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?
+ 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.
I stopped reviewing here, sorry :(
greg k-h
[JDM] My bad on the return values. I will fix them accordingly and
resubmit.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg,
Thanks for your review. I've added some comments inline above for you
to respond to.
Joe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
