Messages in this thread | | | Date | Wed, 2 Nov 2022 09:37:49 -0700 | Subject | Re: [PATCH] scsi: lpfc: fix double free bug in lpfc_bsg_write_ebuf_set | From | James Smart <> |
| |
On 10/27/2022 10:07 PM, Zheng Wang wrote: > When error occurs, it frees dmabuf in both lpfc_bsg_write_ebuf_set > and lpfc_bsg_issue_mbox. > > Fix it by removing free code in lpfc_bsg_write_ebuf_set. > > Reported-by: Zheng Wang <hackerzheng666@gmail.com> > Reported-by: Zhuorao Yang <alex000young@gmail.com> > > Fixes: 7ad20aa9d39a ("[SCSI] lpfc 8.3.24: Extend BSG infrastructure and add link diagnostics") > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > --- > drivers/scsi/lpfc/lpfc_bsg.c | 17 +++-------------- > 1 file changed, 3 insertions(+), 14 deletions(-) > > diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c > index ac0c7ccf2eae..7362d9c1a50b 100644 > --- a/drivers/scsi/lpfc/lpfc_bsg.c > +++ b/drivers/scsi/lpfc/lpfc_bsg.c > @@ -4439,15 +4439,13 @@ lpfc_bsg_write_ebuf_set(struct lpfc_hba *phba, struct bsg_job *job, > > dd_data = kmalloc(sizeof(struct bsg_job_data), GFP_KERNEL); > if (!dd_data) { > - rc = -ENOMEM; > - goto job_error; > + return -ENOMEM; > } > > /* mailbox command structure for base driver */ > pmboxq = mempool_alloc(phba->mbox_mem_pool, GFP_KERNEL); > if (!pmboxq) { > - rc = -ENOMEM; > - goto job_error; > + return -ENOMEM; > } > memset(pmboxq, 0, sizeof(LPFC_MBOXQ_t)); > pbuf = (uint8_t *)phba->mbox_ext_buf_ctx.mbx_dmabuf->virt;
Minimally, just looking at this one snippet, by returning after the mempool_alloc() failure, we are leaking the dd_data memory just allocated.
> @@ -4480,8 +4478,7 @@ lpfc_bsg_write_ebuf_set(struct lpfc_hba *phba, struct bsg_job *job, > lpfc_printf_log(phba, KERN_ERR, LOG_LIBDFC, > "2970 Failed to issue SLI_CONFIG ext-buffer " > "mailbox command, rc:x%x\n", rc); > - rc = -EPIPE; > - goto job_error; > + return -EPIPE;
and this leaks both the dd_data and pmboxq memory.
> } > > /* wait for additional external buffers */ > @@ -4489,14 +4486,6 @@ lpfc_bsg_write_ebuf_set(struct lpfc_hba *phba, struct bsg_job *job, > bsg_job_done(job, bsg_reply->result, > bsg_reply->reply_payload_rcv_len); > return SLI_CONFIG_HANDLED; > - > -job_error: > - if (pmboxq) > - mempool_free(pmboxq, phba->mbox_mem_pool); > - lpfc_bsg_dma_page_free(phba, dmabuf); > - kfree(dd_data); > - > - return rc; > } > > /**
all of these errors should cause: lpfc_bsg_write_ebuf_set() to return -Exxx causing lpfc_bsg_handle_sli_cfg_ebuf() to return -Exxx causing lpfc_bsg_handle_sli_cfg_ext() to return -Exxx which causes lpfc_bsg_issue_mbox() to jump to job_done
I understand the argument is that issue_mbox deletes them, but....
job_done: checks/frees pmboxq is allocated after the jump so it will be NULL frees dmabuf - which was allocated prior to the jump; is freed in freedlpfc_bsg_handle_sli_cfg_ebuf() but only in a block that returns SLI_CONFIG_HANDLED, which is not the block that invokes lpfc_bsg_write_ebuf_set. So it's valid to delete. Note: there's a special case for SLI_CONFIG_HANDLED which skips over these deletes so it's ok. frees dd_data - which is allocated after the jump so it too will be NULL
So - the code is fine. The SLI_CONFIG_HANDLED is a little weird, but the logic is fine. If the patch were added it would leak memory.
I take it this was identified by some tool ?
-- james
| |