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@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]
??? 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@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
