Re: [PATCH v1 1/1] drivers/usb/storage: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux