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.moria...@oracle.com> > > > > > > > Reviewed-by: Steven Sistare <steven.sist...@oracle.com> > > > > > > > Acked-by: Hakon Bugge <hakon.bu...@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(); > > > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
