Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751899AbdGFOpN (ORCPT ); Thu, 6 Jul 2017 10:45:13 -0400 Received: from iolanthe.rowland.org ([192.131.102.54]:60062 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1750896AbdGFOpL (ORCPT ); Thu, 6 Jul 2017 10:45:11 -0400 Date: Thu, 6 Jul 2017 10:45:10 -0400 (EDT) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Colin King cc: Greg Kroah-Hartman , , , , Subject: Re: [PATCH] usb: storage: return on error to avoid a null pointer dereference In-Reply-To: <20170706100635.16610-1-colin.king@canonical.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1394 Lines: 44 On Thu, 6 Jul 2017, Colin King wrote: > From: Colin Ian King > > When us->extra is null the driver is not initialized, however, a > later call to osd200_scsi_to_ata is made that dereferences > us->extra, causing a null pointer dereference. The code > currently detects and reports that the driver is not initialized; > add a return to avoid the subsequent dereference issue in this > check. > > Detected by CoverityScan, CID#100308 ("Dereference after null check") > > Signed-off-by: Colin Ian King > --- > drivers/usb/storage/isd200.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c > index fba4005dd737..174ad60679e7 100644 > --- a/drivers/usb/storage/isd200.c > +++ b/drivers/usb/storage/isd200.c > @@ -1529,8 +1529,10 @@ static void isd200_ata_command(struct scsi_cmnd *srb, struct us_data *us) > > /* Make sure driver was initialized */ > > - if (us->extra == NULL) > + if (us->extra == NULL) { > usb_stor_dbg(us, "ERROR Driver not initialized\n"); > + return; > + } Good catch, but before returning you need to set srb->result = DID_ERROR << 16; so that the SCSI layer will realize the command wasn't handled. Alan Stern > > scsi_set_resid(srb, 0); > /* scsi_bufflen might change in protocol translation to ata */ >