Message ID | 20200112210846.13421-1-bvanassche@acm.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | qla2xxx: Fix a NULL pointer dereference in an error path | expand |
Hi, On Sun, Jan 12, 2020 at 01:08:46PM -0800, Bart Van Assche wrote: > This patch fixes the following Coverity complaint: > > FORWARD_NULL > > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > >>> CID 353340: (FORWARD_NULL) > >>> Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > >>> CID 353340: (FORWARD_NULL) > >>> Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } nitpick: one copy of the coverity report is enough. > Cc: Himanshu Madhani <hmadhani@marvell.com> > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Fixes: 3dae220595ba ("scsi: qla2xxx: Use common routine to free fcport struct") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Daniel Wagner <dwagner@suse.de> Thanks, Daniel
On Sun, 2020-01-12 at 13:08 -0800, Bart Van Assche wrote: > This patch fixes the following Coverity complaint: > > FORWARD_NULL > > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > > > > CID 353340: (FORWARD_NULL) > > > > Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > > > > CID 353340: (FORWARD_NULL) > > > > Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } > > Cc: Himanshu Madhani <hmadhani@marvell.com> > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Fixes: 3dae220595ba ("scsi: qla2xxx: Use common routine to free fcport struct") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/qla2xxx/qla_init.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index c4e087217484..6560908ed50e 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -4895,6 +4895,8 @@ qla2x00_alloc_fcport(scsi_qla_host_t *vha, gfp_t flags) > void > qla2x00_free_fcport(fc_port_t *fcport) > { > + if (!fcport) > + return; > if (fcport->ct_desc.ct_sns) { > dma_free_coherent(&fcport->vha->hw->pdev->dev, > sizeof(struct ct_sns_pkt), fcport->ct_desc.ct_sns, > I would have fixed this by moving the label to be after the qla2x00_free_fcport() call in qla2x00_configure_local_loop(), which Coverity complained about. And called it something different. (The code could probably be simplified by only making a call to qla2x00_alloc_fcport() in one place, something to think about...) I also notice that there is duplicate code in qla2x00_alloc_fcport() that tests for: if (!fcport->ct_desc.ct_sns) But, this should fix the Coverity issue. Reviewed-by: Ewan D. Milne <emilne@redhat.com>
On 1/13/20 7:29 AM, Ewan D. Milne wrote: > On Sun, 2020-01-12 at 13:08 -0800, Bart Van Assche wrote: >> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c >> index c4e087217484..6560908ed50e 100644 >> --- a/drivers/scsi/qla2xxx/qla_init.c >> +++ b/drivers/scsi/qla2xxx/qla_init.c >> @@ -4895,6 +4895,8 @@ qla2x00_alloc_fcport(scsi_qla_host_t *vha, gfp_t flags) >> void >> qla2x00_free_fcport(fc_port_t *fcport) >> { >> + if (!fcport) >> + return; >> if (fcport->ct_desc.ct_sns) { >> dma_free_coherent(&fcport->vha->hw->pdev->dev, >> sizeof(struct ct_sns_pkt), fcport->ct_desc.ct_sns, >> > > I would have fixed this by moving the label to be after the qla2x00_free_fcport() > call in qla2x00_configure_local_loop(), which Coverity complained about. And > called it something different. (The code could probably be simplified by only > making a call to qla2x00_alloc_fcport() in one place, something to think about...) Hi Ewan, I have considered the solution that you proposed. The solution shown above was chosen because I did not want to introduce any memory leaks in qla2x00_configure_local_loop(). There is a "goto cleanup_allocation" statement in that function that occurs after the "new_fcport = qla2x00_alloc_fcport(vha, GFP_KERNEL)" assignment. That is hard to notice because the qla2x00_configure_local_loop() function is so long. Thanks, Bart.
On Sun, Jan 12, 2020 at 01:08:46PM -0800, Bart Van Assche wrote: > This patch fixes the following Coverity complaint: > > FORWARD_NULL > > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > >>> CID 353340: (FORWARD_NULL) > >>> Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } > > --- > drivers/scsi/qla2xxx/qla_init.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index c4e087217484..6560908ed50e 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -4895,6 +4895,8 @@ qla2x00_alloc_fcport(scsi_qla_host_t *vha, gfp_t flags) > void > qla2x00_free_fcport(fc_port_t *fcport) > { > + if (!fcport) > + return; > if (fcport->ct_desc.ct_sns) { > dma_free_coherent(&fcport->vha->hw->pdev->dev, > sizeof(struct ct_sns_pkt), fcport->ct_desc.ct_sns, Hi Bart, Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com> There was another attempt to fix the issue a week ago: https://patchwork.kernel.org/patch/11319315/ CC'ing Colin. Thanks, Roman
On Mon, 2020-01-13 at 08:11 -0800, Bart Van Assche wrote: > On 1/13/20 7:29 AM, Ewan D. Milne wrote: > > On Sun, 2020-01-12 at 13:08 -0800, Bart Van Assche wrote: > > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > > > index c4e087217484..6560908ed50e 100644 > > > --- a/drivers/scsi/qla2xxx/qla_init.c > > > +++ b/drivers/scsi/qla2xxx/qla_init.c > > > @@ -4895,6 +4895,8 @@ qla2x00_alloc_fcport(scsi_qla_host_t *vha, gfp_t flags) > > > void > > > qla2x00_free_fcport(fc_port_t *fcport) > > > { > > > + if (!fcport) > > > + return; > > > if (fcport->ct_desc.ct_sns) { > > > dma_free_coherent(&fcport->vha->hw->pdev->dev, > > > sizeof(struct ct_sns_pkt), fcport->ct_desc.ct_sns, > > > > > > > I would have fixed this by moving the label to be after the qla2x00_free_fcport() > > call in qla2x00_configure_local_loop(), which Coverity complained about. And > > called it something different. (The code could probably be simplified by only > > making a call to qla2x00_alloc_fcport() in one place, something to think about...) > > Hi Ewan, > > I have considered the solution that you proposed. The solution shown > above was chosen because I did not want to introduce any memory leaks in > qla2x00_configure_local_loop(). There is a "goto cleanup_allocation" > statement in that function that occurs after the "new_fcport = > qla2x00_alloc_fcport(vha, GFP_KERNEL)" assignment. That is hard to > notice because the qla2x00_configure_local_loop() function is so long. > Yes, but isn't that after "if (new_fcport == NULL)" where the code has put the previously allocated fcport into the &vha->vp_fcports list and was unable to allocate another one? -Ewan
On 2020-01-14 10:13, Ewan D. Milne wrote: > Yes, but isn't that after "if (new_fcport == NULL)" where the code has > put the previously allocated fcport into the &vha->vp_fcports list and > was unable to allocate another one? How about the (untested) patch below? Thanks, Bart. From 436e1552f79b3a3b7d3f3b1dea1df27c33bd0d49 Mon Sep 17 00:00:00 2001 From: Bart Van Assche <bvanassche@acm.org> Date: Sun, 12 Jan 2020 09:17:37 -0800 Subject: [PATCH v2] qla2xxx: Fix a NULL pointer dereference in an error path This patch fixes the following Coverity complaint: FORWARD_NULL qla_init.c: 5275 in qla2x00_configure_local_loop() 5269 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) 5271 qla24xx_fcport_handle_login(vha, fcport); 5272 } 5273 5274 cleanup_allocation: >>> CID 353340: (FORWARD_NULL) >>> Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. 5275 qla2x00_free_fcport(new_fcport); 5276 5277 if (rval != QLA_SUCCESS) { 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, 5279 "Configure local loop error exit: rval=%x.\n", rval); 5280 } qla_init.c: 5275 in qla2x00_configure_local_loop() 5269 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) 5271 qla24xx_fcport_handle_login(vha, fcport); 5272 } 5273 5274 cleanup_allocation: >>> CID 353340: (FORWARD_NULL) >>> Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. 5275 qla2x00_free_fcport(new_fcport); 5276 5277 if (rval != QLA_SUCCESS) { 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, 5279 "Configure local loop error exit: rval=%x.\n", rval); 5280 } Cc: Himanshu Madhani <hmadhani@marvell.com> Cc: Quinn Tran <qutran@marvell.com> Cc: Martin Wilck <mwilck@suse.com> Cc: Daniel Wagner <dwagner@suse.de> Cc: Roman Bolshakov <r.bolshakov@yadro.com> Fixes: 3dae220595ba ("scsi: qla2xxx: Use common routine to free fcport struct") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/qla2xxx/qla_init.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index c4e087217484..62df78258269 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -5109,7 +5109,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) rval = qla2x00_get_id_list(vha, ha->gid_list, ha->gid_list_dma, &entries); if (rval != QLA_SUCCESS) - goto cleanup_allocation; + goto err; ql_dbg(ql_dbg_disc, vha, 0x2011, "Entries in ID list (%d).\n", entries); @@ -5139,7 +5139,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) ql_log(ql_log_warn, vha, 0x2012, "Memory allocation failed for fcport.\n"); rval = QLA_MEMORY_ALLOC_FAILED; - goto cleanup_allocation; + goto err; } new_fcport->flags &= ~FCF_FABRIC_DEVICE; @@ -5229,7 +5229,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) ql_log(ql_log_warn, vha, 0xd031, "Failed to allocate memory for fcport.\n"); rval = QLA_MEMORY_ALLOC_FAILED; - goto cleanup_allocation; + goto err; } spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags); new_fcport->flags &= ~FCF_FABRIC_DEVICE; @@ -5272,15 +5272,14 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) qla24xx_fcport_handle_login(vha, fcport); } -cleanup_allocation: qla2x00_free_fcport(new_fcport); - if (rval != QLA_SUCCESS) { - ql_dbg(ql_dbg_disc, vha, 0x2098, - "Configure local loop error exit: rval=%x.\n", rval); - } + return rval; - return (rval); +err: + ql_dbg(ql_dbg_disc, vha, 0x2098, + "Configure local loop error exit: rval=%x.\n", rval); + return rval; } static void
On Tue, 2020-01-14 at 18:56 -0800, Bart Van Assche wrote: > On 2020-01-14 10:13, Ewan D. Milne wrote: > > Yes, but isn't that after "if (new_fcport == NULL)" where the code has > > put the previously allocated fcport into the &vha->vp_fcports list and > > was unable to allocate another one? > > How about the (untested) patch below? > > Thanks, > > Bart. > > > From 436e1552f79b3a3b7d3f3b1dea1df27c33bd0d49 Mon Sep 17 00:00:00 2001 > From: Bart Van Assche <bvanassche@acm.org> > Date: Sun, 12 Jan 2020 09:17:37 -0800 > Subject: [PATCH v2] qla2xxx: Fix a NULL pointer dereference in an error path > > This patch fixes the following Coverity complaint: > > FORWARD_NULL > > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > > > > CID 353340: (FORWARD_NULL) > > > > Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } > qla_init.c: 5275 in qla2x00_configure_local_loop() > 5269 > 5270 if (fcport->scan_state == QLA_FCPORT_FOUND) > 5271 qla24xx_fcport_handle_login(vha, fcport); > 5272 } > 5273 > 5274 cleanup_allocation: > > > > CID 353340: (FORWARD_NULL) > > > > Passing null pointer "new_fcport" to "qla2x00_free_fcport", which dereferences it. > > 5275 qla2x00_free_fcport(new_fcport); > 5276 > 5277 if (rval != QLA_SUCCESS) { > 5278 ql_dbg(ql_dbg_disc, vha, 0x2098, > 5279 "Configure local loop error exit: rval=%x.\n", rval); > 5280 } > > Cc: Himanshu Madhani <hmadhani@marvell.com> > Cc: Quinn Tran <qutran@marvell.com> > Cc: Martin Wilck <mwilck@suse.com> > Cc: Daniel Wagner <dwagner@suse.de> > Cc: Roman Bolshakov <r.bolshakov@yadro.com> > Fixes: 3dae220595ba ("scsi: qla2xxx: Use common routine to free fcport struct") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/qla2xxx/qla_init.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index c4e087217484..62df78258269 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -5109,7 +5109,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) > rval = qla2x00_get_id_list(vha, ha->gid_list, ha->gid_list_dma, > &entries); > if (rval != QLA_SUCCESS) > - goto cleanup_allocation; > + goto err; > > ql_dbg(ql_dbg_disc, vha, 0x2011, > "Entries in ID list (%d).\n", entries); > @@ -5139,7 +5139,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) > ql_log(ql_log_warn, vha, 0x2012, > "Memory allocation failed for fcport.\n"); > rval = QLA_MEMORY_ALLOC_FAILED; > - goto cleanup_allocation; > + goto err; > } > new_fcport->flags &= ~FCF_FABRIC_DEVICE; > > @@ -5229,7 +5229,7 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) > ql_log(ql_log_warn, vha, 0xd031, > "Failed to allocate memory for fcport.\n"); > rval = QLA_MEMORY_ALLOC_FAILED; > - goto cleanup_allocation; > + goto err; > } > spin_lock_irqsave(&vha->hw->tgt.sess_lock, flags); > new_fcport->flags &= ~FCF_FABRIC_DEVICE; > @@ -5272,15 +5272,14 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha) > qla24xx_fcport_handle_login(vha, fcport); > } > > -cleanup_allocation: > qla2x00_free_fcport(new_fcport); > > - if (rval != QLA_SUCCESS) { > - ql_dbg(ql_dbg_disc, vha, 0x2098, > - "Configure local loop error exit: rval=%x.\n", rval); > - } > + return rval; > > - return (rval); > +err: > + ql_dbg(ql_dbg_disc, vha, 0x2098, > + "Configure local loop error exit: rval=%x.\n", rval); > + return rval; > } > > static void > That looks fine. Thanks. Reviewed-by: Ewan D. Milne <emilne@redhat.com>
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index c4e087217484..6560908ed50e 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -4895,6 +4895,8 @@ qla2x00_alloc_fcport(scsi_qla_host_t *vha, gfp_t flags) void qla2x00_free_fcport(fc_port_t *fcport) { + if (!fcport) + return; if (fcport->ct_desc.ct_sns) { dma_free_coherent(&fcport->vha->hw->pdev->dev, sizeof(struct ct_sns_pkt), fcport->ct_desc.ct_sns,