| Message ID | 1643289172-165636-4-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: pm8001: Documentation and use-after-free fixes | expand |
On 1/27/22 22:12, John Garry wrote: > Currently a use-after-free may occur if a sas_task is aborted by the upper > layer before we handle the IO completion in mpi_ssp_completion() or > mpi_sata_completion(). > > In this case, the following are the two steps in handling those IO > completions: > - call complete() to inform the upper layer handler of completion of > the IO > - release driver resources associated with the sas_task in > pm8001_ccb_task_free() call > > When complete() is called, the upper layer may free the sas_task. As such, > we should not touch the associated sas_task afterwards, but we do so in > the pm8001_ccb_task_free() call. > > Fix by swapping the complete() and pm8001_ccb_task_free() calls ordering. > > Signed-off-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/pm8001/pm80xx_hwi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c > index ce38a2298e75..1134e86ac928 100644 > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > @@ -2185,9 +2185,9 @@ mpi_ssp_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) > pm8001_dbg(pm8001_ha, FAIL, > "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, status, ts->resp, ts->stat); > + pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > if (t->slow_task) > complete(&t->slow_task->completion); > - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else { > spin_unlock_irqrestore(&t->task_state_lock, flags); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > @@ -2794,9 +2794,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, > pm8001_dbg(pm8001_ha, FAIL, > "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, status, ts->resp, ts->stat); > + pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > if (t->slow_task) > complete(&t->slow_task->completion); > - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else { > spin_unlock_irqrestore(&t->task_state_lock, flags); > spin_unlock_irqrestore(&circularQ->oq_lock, Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
On Thu, Jan 27, 2022 at 2:18 PM John Garry <john.garry@huawei.com> wrote: > > Currently a use-after-free may occur if a sas_task is aborted by the upper > layer before we handle the IO completion in mpi_ssp_completion() or > mpi_sata_completion(). > > In this case, the following are the two steps in handling those IO > completions: > - call complete() to inform the upper layer handler of completion of > the IO > - release driver resources associated with the sas_task in > pm8001_ccb_task_free() call > > When complete() is called, the upper layer may free the sas_task. As such, > we should not touch the associated sas_task afterwards, but we do so in > the pm8001_ccb_task_free() call. > > Fix by swapping the complete() and pm8001_ccb_task_free() calls ordering. > > Signed-off-by: John Garry <john.garry@huawei.com> Thx John! Acked-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/scsi/pm8001/pm80xx_hwi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c > index ce38a2298e75..1134e86ac928 100644 > --- a/drivers/scsi/pm8001/pm80xx_hwi.c > +++ b/drivers/scsi/pm8001/pm80xx_hwi.c > @@ -2185,9 +2185,9 @@ mpi_ssp_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) > pm8001_dbg(pm8001_ha, FAIL, > "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, status, ts->resp, ts->stat); > + pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > if (t->slow_task) > complete(&t->slow_task->completion); > - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else { > spin_unlock_irqrestore(&t->task_state_lock, flags); > pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > @@ -2794,9 +2794,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, > pm8001_dbg(pm8001_ha, FAIL, > "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", > t, status, ts->resp, ts->stat); > + pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > if (t->slow_task) > complete(&t->slow_task->completion); > - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); > } else { > spin_unlock_irqrestore(&t->task_state_lock, flags); > spin_unlock_irqrestore(&circularQ->oq_lock, > -- > 2.26.2 >
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c index ce38a2298e75..1134e86ac928 100644 --- a/drivers/scsi/pm8001/pm80xx_hwi.c +++ b/drivers/scsi/pm8001/pm80xx_hwi.c @@ -2185,9 +2185,9 @@ mpi_ssp_completion(struct pm8001_hba_info *pm8001_ha, void *piomb) pm8001_dbg(pm8001_ha, FAIL, "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", t, status, ts->resp, ts->stat); + pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); if (t->slow_task) complete(&t->slow_task->completion); - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } else { spin_unlock_irqrestore(&t->task_state_lock, flags); pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); @@ -2794,9 +2794,9 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, pm8001_dbg(pm8001_ha, FAIL, "task 0x%p done with io_status 0x%x resp 0x%x stat 0x%x but aborted by upper layer!\n", t, status, ts->resp, ts->stat); + pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); if (t->slow_task) complete(&t->slow_task->completion); - pm8001_ccb_task_free(pm8001_ha, t, ccb, tag); } else { spin_unlock_irqrestore(&t->task_state_lock, flags); spin_unlock_irqrestore(&circularQ->oq_lock,
Currently a use-after-free may occur if a sas_task is aborted by the upper layer before we handle the IO completion in mpi_ssp_completion() or mpi_sata_completion(). In this case, the following are the two steps in handling those IO completions: - call complete() to inform the upper layer handler of completion of the IO - release driver resources associated with the sas_task in pm8001_ccb_task_free() call When complete() is called, the upper layer may free the sas_task. As such, we should not touch the associated sas_task afterwards, but we do so in the pm8001_ccb_task_free() call. Fix by swapping the complete() and pm8001_ccb_task_free() calls ordering. Signed-off-by: John Garry <john.garry@huawei.com> --- drivers/scsi/pm8001/pm80xx_hwi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)