[PATCH v2] lpfc: Fix possible NULL pointer dereference

[PATCH v2] lpfc: Fix possible NULL pointer dereference

[Johannes Thumshirn]

Check for the existence of piocb->vport before accessing it.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
Changes to v1:
* Fix spelling of piocb (Tyrel)
* re-introduce the BU() assertion if piocb->vport is NULL (Tyrel)


 drivers/scsi/lpfc/lpfc_sli.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 70edf21..3d04ef1 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -1323,21 +1323,18 @@ lpfc_sli_ringtxcmpl_put(struct lpfc_hba *phba, struct lpfc_sli_ring *pring,
 {
 	lockdep_assert_held(&phba->hbalock);
 
+	BUG_ON(!piocb || !piocb->vport);
+
 	list_add_tail(&piocb->list, &pring->txcmplq);
 	piocb->iocb_flag |= LPFC_IO_ON_TXCMPLQ;
 
 	if ((unlikely(pring->ringno == LPFC_ELS_RING)) &&
 	   (piocb->iocb.ulpCommand != CMD_ABORT_XRI_CN) &&
 	   (piocb->iocb.ulpCommand != CMD_CLOSE_XRI_CN) &&
-	 (!(piocb->vport->load_flag & FC_UNLOADING))) {
-		if (!piocb->vport)
-			BUG();
-		else
-			mod_timer(&piocb->vport->els_tmofunc,
-				jiffies +
-				msecs_to_jiffies(1000 * (phba->fc_ratov << 1)));
-	}
-
+	    (!(piocb->vport->load_flag & FC_UNLOADING)))
+		mod_timer(&piocb->vport->els_tmofunc,
+			  jiffies +
+			  msecs_to_jiffies(1000 * (phba->fc_ratov << 1)));
 
 	return 0;
 }
-- 
1.8.5.6

Re: [PATCH v2] lpfc: Fix possible NULL pointer dereference

[Tyrel Datwyler]

On 07/29/2016 06:30 AM, Johannes Thumshirn wrote:
> Check for the existence of piocb->vport before accessing it.
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>

Reviewed-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>

Re: [PATCH v2] lpfc: Fix possible NULL pointer dereference

[Martin K. Petersen]

>>>>> "Johannes" == Johannes Thumshirn <jthumshirn@suse.de> writes:

Johannes> Check for the existence of piocb->vport before accessing it.

Applied to 4.8/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] lpfc: Fix possible NULL pointer dereference

[James Bottomley]

On Tue, 2016-08-02 at 01:17 -0400, Martin K. Petersen wrote:
> > > > > > "Johannes" == Johannes Thumshirn <jthumshirn@suse.de>
> > > > > > writes:
> 
> Johannes> Check for the existence of piocb->vport before accessing
> it.
> 
> Applied to 4.8/scsi-queue.

OK, now that this has caused problems, could learn the lessons from it?

Lines like this:

+       BUG_ON(!piocb || !piocb->vport);

Should never appear in code.  They only have the potential to cause
problems if the condition is inexact and they provide precisely no
information over what a NULL deref in the kernel is going to tell us
anyway ... this one even obscures information because you don't know if
pciob was null or pciob->vport when it triggers.

The rule is never BUG_ON a NULL pointer unless you have an extremely
good reason why the kernel NULL deref handler isn't adequate (which
should be documented in the commit log).

James

Re: [PATCH v2] lpfc: Fix possible NULL pointer dereference

[Johannes Thumshirn]

On Thu, Dec 08, 2016 at 11:01:30AM -0800, James Bottomley wrote:
> On Tue, 2016-08-02 at 01:17 -0400, Martin K. Petersen wrote:
> > > > > > > "Johannes" == Johannes Thumshirn <jthumshirn@suse.de>
> > > > > > > writes:
> > 
> > Johannes> Check for the existence of piocb->vport before accessing
> > it.
> > 
> > Applied to 4.8/scsi-queue.
> 
> OK, now that this has caused problems, could learn the lessons from it?
> 
> Lines like this:
> 
> +       BUG_ON(!piocb || !piocb->vport);
> 
> Should never appear in code.  They only have the potential to cause
> problems if the condition is inexact and they provide precisely no
> information over what a NULL deref in the kernel is going to tell us
> anyway ... this one even obscures information because you don't know if
> pciob was null or pciob->vport when it triggers.
> 
> The rule is never BUG_ON a NULL pointer unless you have an extremely
> good reason why the kernel NULL deref handler isn't adequate (which
> should be documented in the commit log).

Yup I fully agree, but shouldn't we take as a 2nd lesson that BUG_ON()s
generally aren't an extremely good idea? I personally think a lot of
BUG_ON()s in driver code can be eliminated with proper error handling.

That said, mea culpa.

Byte,
	Johannes

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850