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. Why not ask the scsi developers about this? They put that line there for a good reason, right? And if NULL is valid to return, then your patch still does not fix the issue at all, which was my main point. So go ask the linux-scsi list about this either way. 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
