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

From: Ewan Milne <emilne@redhat.com>
To: James Smart <jsmart2021@gmail.com>
Cc: linux-scsi@vger.kernel.org, Justin Tee <justin.tee@broadcom.com>
Subject: Re: [PATCH] lpfc: Fix memory overwrite during FC-GS IO abort handling
Date: Tue, 5 Oct 2021 13:11:44 -0400	[thread overview]
Message-ID: <CAGtn9rmsV9QcMr2-dKR8GEN+Ln7MtaCy5ruY+5gzXoUy+gg3pw@mail.gmail.com> (raw)
In-Reply-To: <20211004231210.35524-1-jsmart2021@gmail.com>

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
>