Re: [patch] potential null deref in lpfc_els.c

Re: [patch] potential null deref in lpfc_els.c

[James Smart]

> This patch is against 2.6.32-rc8.
>
> The issue was found by a static checker.  If cmd==ELS_CMD_PLOGI, it
> is possible for ndlp to be NULL.  We do check ndlp further down the
> function so that would also indicate that we should check ndlp here.
> 
> Compile tested.
>

Dan,

Thanks. We never hit this as it's code specific to using debugfs, which
we don't turn on except for very rare occasions. Anyway, it is an error,
so it's worth correcting.

I've updated your patch to change the default value - I wanted to D_ID
to be a non-valid value.

-- james s



 Signed-off-by: James Smart <james.smart@emulex.com>

 ---

 lpfc_els.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff -upNr a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
--- a/drivers/scsi/lpfc/lpfc_els.c	2009-12-08 09:43:11.000000000 -0500
+++ b/drivers/scsi/lpfc/lpfc_els.c	2009-12-08 12:36:56.000000000 -0500
@@ -2562,7 +2562,8 @@ lpfc_els_retry(struct lpfc_hba *phba, st
 
 	lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_ELS_CMD,
 		"Retry ELS:       wd7:x%x wd4:x%x did:x%x",
-		*(((uint32_t *) irsp) + 7), irsp->un.ulpWord[4], ndlp->nlp_DID);
+		*(((uint32_t *) irsp) + 7), irsp->un.ulpWord[4],
+		(ndlp ?  ndlp->nlp_DID : 0xFFFFFFFF));
 
 	switch (irsp->ulpStatus) {
 	case IOSTAT_FCP_RSP_ERROR:

Re: [patch] potential null deref in lpfc_els.c

[Dan Carpenter]

On Tue, Dec 08, 2009 at 01:01:23PM -0500, James Smart wrote:
> 
> > This patch is against 2.6.32-rc8.
> >
> > The issue was found by a static checker.  If cmd==ELS_CMD_PLOGI, it
> > is possible for ndlp to be NULL.  We do check ndlp further down the
> > function so that would also indicate that we should check ndlp here.
> > 
> > Compile tested.
> >
> 
> Dan,
> 
> Thanks. We never hit this as it's code specific to using debugfs, which
> we don't turn on except for very rare occasions. Anyway, it is an error,
> so it's worth correcting.
> 
> I've updated your patch to change the default value - I wanted to D_ID
> to be a non-valid value.
> 

Cool.  Thanks, I should have asked you what value you wanted there before.

regards,
dan

> -- james s
> 
> 
> 
>  Signed-off-by: James Smart <james.smart@emulex.com>
> 
>  ---
> 
>  lpfc_els.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff -upNr a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> --- a/drivers/scsi/lpfc/lpfc_els.c	2009-12-08 09:43:11.000000000 -0500
> +++ b/drivers/scsi/lpfc/lpfc_els.c	2009-12-08 12:36:56.000000000 -0500
> @@ -2562,7 +2562,8 @@ lpfc_els_retry(struct lpfc_hba *phba, st
>  
>  	lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_ELS_CMD,
>  		"Retry ELS:       wd7:x%x wd4:x%x did:x%x",
> -		*(((uint32_t *) irsp) + 7), irsp->un.ulpWord[4], ndlp->nlp_DID);
> +		*(((uint32_t *) irsp) + 7), irsp->un.ulpWord[4],
> +		(ndlp ?  ndlp->nlp_DID : 0xFFFFFFFF));
>  
>  	switch (irsp->ulpStatus) {
>  	case IOSTAT_FCP_RSP_ERROR:
> 

[patch] potential null deref in lpfc_els.c

[Dan Carpenter]

This patch is against 2.6.32-rc8.

The issue was found by a static checker.  If cmd == ELS_CMD_PLOGI, it
is possible for ndlp to be NULL.  We do check ndlp further down the 
function so that would also indicate that we should check ndlp here.

Compile tested.

Signed-off-by: Dan Carpenter <error27@gmail.com>

--- orig/drivers/scsi/lpfc/lpfc_els.c	2009-12-05 10:52:02.000000000 +0200
+++ devel/drivers/scsi/lpfc/lpfc_els.c	2009-12-05 11:43:49.000000000 +0200
@@ -2548,7 +2548,7 @@ lpfc_els_retry(struct lpfc_hba *phba, st
 
 	lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_ELS_CMD,
 		"Retry ELS:       wd7:x%x wd4:x%x did:x%x",
-		*(((uint32_t *) irsp) + 7), irsp->un.ulpWord[4], ndlp->nlp_DID);
+		*(((uint32_t *) irsp) + 7), irsp->un.ulpWord[4], (ndlp ? ndlp->nlp_DID : 0));
 
 	switch (irsp->ulpStatus) {
 	case IOSTAT_FCP_RSP_ERROR: