[PATCH] lpfc: Fix memory overwrite during FC-GS IO abort handling

[PATCH] lpfc: Fix memory overwrite during FC-GS IO abort handling

[James Smart]

When an FC-GS IO is aborted by lpfc, the driver requires a node pointer
for a dereference operation.  In the abort IO routine, the driver
miscasts a context pointer to the wrong data type and overwrites a
single byte outside of the allocated space.  This miscast is done in the
abort io function handler because the abort io handler works on FC-GS
and FC-LS commands but the code neglected to get the correct job location
for the node.

Fix this by acquiring the necessary node pointer from the correct
job structure depending on the IO type.

Co-developed-by: Justin Tee <justin.tee@broadcom.com>
Signed-off-by: Justin Tee <justin.tee@broadcom.com>
Signed-off-by: James Smart <jsmart2021@gmail.com>
---
 drivers/scsi/lpfc/lpfc_sli.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 3f911cb48cf2..d8c01114442f 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -12308,12 +12308,12 @@ void
 lpfc_ignore_els_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
 		     struct lpfc_iocbq *rspiocb)
 {
-	struct lpfc_nodelist *ndlp = (struct lpfc_nodelist *) cmdiocb->context1;
+	struct lpfc_nodelist *ndlp = NULL;
 	IOCB_t *irsp = &rspiocb->iocb;
 
 	/* ELS cmd tag <ulpIoTag> completes */
 	lpfc_printf_log(phba, KERN_INFO, LOG_ELS,
-			"0139 Ignoring ELS cmd tag x%x completion Data: "
+			"0139 Ignoring ELS cmd code x%x completion Data: "
 			"x%x x%x x%x\n",
 			irsp->ulpIoTag, irsp->ulpStatus,
 			irsp->un.ulpWord[4], irsp->ulpTimeout);
@@ -12321,10 +12321,13 @@ lpfc_ignore_els_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
 	 * Deref the ndlp after free_iocb. sli_release_iocb will access the ndlp
 	 * if exchange is busy.
 	 */
-	if (cmdiocb->iocb.ulpCommand == CMD_GEN_REQUEST64_CR)
+	if (cmdiocb->iocb.ulpCommand == CMD_GEN_REQUEST64_CR) {
+		ndlp = cmdiocb->context_un.ndlp;
 		lpfc_ct_free_iocb(phba, cmdiocb);
-	else
+	} else {
+		ndlp = (struct lpfc_nodelist *) cmdiocb->context1;
 		lpfc_els_free_iocb(phba, cmdiocb);
+	}
 
 	lpfc_nlp_put(ndlp);
 }
-- 
2.26.2

Re: [PATCH] lpfc: Fix memory overwrite during FC-GS IO abort handling

[Martin K. Petersen]

On Mon, 4 Oct 2021 16:12:10 -0700, James Smart wrote:

> When an FC-GS IO is aborted by lpfc, the driver requires a node pointer
> for a dereference operation.  In the abort IO routine, the driver
> miscasts a context pointer to the wrong data type and overwrites a
> single byte outside of the allocated space.  This miscast is done in the
> abort io function handler because the abort io handler works on FC-GS
> and FC-LS commands but the code neglected to get the correct job location
> for the node.
> 
> [...]

Applied to 5.15/scsi-fixes, thanks!

[1/1] lpfc: Fix memory overwrite during FC-GS IO abort handling
      https://git.kernel.org/mkp/scsi/c/69a3a7bc7239

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] lpfc: Fix memory overwrite during FC-GS IO abort handling

[Ewan Milne]

Tested-by: Ewan D. Milne <emilne@redhat.com>

On Mon, Oct 4, 2021 at 7:12 PM James Smart <jsmart2021@gmail.com> wrote:
>
> When an FC-GS IO is aborted by lpfc, the driver requires a node pointer
> for a dereference operation.  In the abort IO routine, the driver
> miscasts a context pointer to the wrong data type and overwrites a
> single byte outside of the allocated space.  This miscast is done in the
> abort io function handler because the abort io handler works on FC-GS
> and FC-LS commands but the code neglected to get the correct job location
> for the node.
>
> Fix this by acquiring the necessary node pointer from the correct
> job structure depending on the IO type.
>
> Co-developed-by: Justin Tee <justin.tee@broadcom.com>
> Signed-off-by: Justin Tee <justin.tee@broadcom.com>
> Signed-off-by: James Smart <jsmart2021@gmail.com>
> ---
>  drivers/scsi/lpfc/lpfc_sli.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
> index 3f911cb48cf2..d8c01114442f 100644
> --- a/drivers/scsi/lpfc/lpfc_sli.c
> +++ b/drivers/scsi/lpfc/lpfc_sli.c
> @@ -12308,12 +12308,12 @@ void
>  lpfc_ignore_els_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>                      struct lpfc_iocbq *rspiocb)
>  {
> -       struct lpfc_nodelist *ndlp = (struct lpfc_nodelist *) cmdiocb->context1;
> +       struct lpfc_nodelist *ndlp = NULL;
>         IOCB_t *irsp = &rspiocb->iocb;
>
>         /* ELS cmd tag <ulpIoTag> completes */
>         lpfc_printf_log(phba, KERN_INFO, LOG_ELS,
> -                       "0139 Ignoring ELS cmd tag x%x completion Data: "
> +                       "0139 Ignoring ELS cmd code x%x completion Data: "
>                         "x%x x%x x%x\n",
>                         irsp->ulpIoTag, irsp->ulpStatus,
>                         irsp->un.ulpWord[4], irsp->ulpTimeout);
> @@ -12321,10 +12321,13 @@ lpfc_ignore_els_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
>          * Deref the ndlp after free_iocb. sli_release_iocb will access the ndlp
>          * if exchange is busy.
>          */
> -       if (cmdiocb->iocb.ulpCommand == CMD_GEN_REQUEST64_CR)
> +       if (cmdiocb->iocb.ulpCommand == CMD_GEN_REQUEST64_CR) {
> +               ndlp = cmdiocb->context_un.ndlp;
>                 lpfc_ct_free_iocb(phba, cmdiocb);
> -       else
> +       } else {
> +               ndlp = (struct lpfc_nodelist *) cmdiocb->context1;
>                 lpfc_els_free_iocb(phba, cmdiocb);
> +       }
>
>         lpfc_nlp_put(ndlp);
>  }
> --
> 2.26.2
>