On 2/27/2018 2:05 PM, Greg KH wrote:
On Tue, Feb 27, 2018 at 01:22:24PM -0500, Joe Moriarty wrote:
On 2/27/2018 1:14 PM, Greg KH wrote:
On Tue, Feb 27, 2018 at 09:59:40AM -0500, Joe Moriarty wrote:
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();
No, never crash the kernel.
There is Bug() throughout the kernel. So that is invalid.
Nope, we are trying to get rid of them. If you see new ones being
added, please point it out.
Kernel Developers will put in Bug() statements anywhere the code is
not suppose to go. It is easier to debug and then trying to backtrace
from a side effect issue.
Not at all, it causes the machine to crash and make it harder to debug.
Or it causes a reboot. Even WARN_ON() should not be used for debugging
as we are seeing tools hit those and reboot and complain (see the
syzkaller threads about this.)
If you want a debugging message, make it a debugging message using
dev_dbg(). That's all that is needed.
Why not ask the scsi developers about this? They put that line there
for a good reason, right?
I submitted the original patch here because the original patch was in the
drivers/usb/storage directory of which you are the maintainer.
But this is a scsi core api call, I know nothing about that :)
And if NULL is valid to return, then your patch still does not fix the
issue at all, which was my main point.
Yes it does. A NULL return from dev_to_shost will kernel panic the machine
in multiple places in the SCSI kernel modules. Swapping out the NULL with a
BUG() will do that.
Neither is good to have, they should all be fixed.
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
Ok, Then I will need to stay in the linux-usb group to fix the NULL
pointer dereference problem. The fix will need to be applied to
drivers/usb/storage/scsiglue.c and *NOT* include/scsi/scsi_host.h with
the call to BUG(). You can't get rid of me that easily :-). I'll send
a new version out in the next few days for review.
Thanks,
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
