* [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
@ 2020-01-18 4:20 Bart Van Assche
2020-01-20 8:58 ` Daniel Wagner
2020-01-20 23:38 ` Martin K. Petersen
0 siblings, 2 replies; 11+ messages in thread
From: Bart Van Assche @ 2020-01-18 4:20 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Himanshu Madhani, Quinn Tran,
Martin Wilck, Daniel Wagner, Roman Bolshakov, Ewan D . Milne
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>
Reviewed-by: Ewan D. Milne <emilne@redhat.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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-18 4:20 [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path Bart Van Assche
@ 2020-01-20 8:58 ` Daniel Wagner
2020-01-20 23:38 ` Martin K. Petersen
1 sibling, 0 replies; 11+ messages in thread
From: Daniel Wagner @ 2020-01-20 8:58 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Himanshu Madhani, Quinn Tran, Martin Wilck, Roman Bolshakov,
Ewan D . Milne
On Fri, Jan 17, 2020 at 08:20:56PM -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>
> Reviewed-by: Ewan D. Milne <emilne@redhat.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>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-18 4:20 [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path Bart Van Assche
2020-01-20 8:58 ` Daniel Wagner
@ 2020-01-20 23:38 ` Martin K. Petersen
1 sibling, 0 replies; 11+ messages in thread
From: Martin K. Petersen @ 2020-01-20 23:38 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Himanshu Madhani, Quinn Tran, Martin Wilck, Daniel Wagner,
Roman Bolshakov, Ewan D . Milne
Bart,
> This patch fixes the following Coverity complaint:
Applied to 5.6/scsi-queue, thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
@ 2020-01-12 21:08 Bart Van Assche
2020-01-13 10:02 ` Daniel Wagner
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Bart Van Assche @ 2020-01-12 21:08 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Bart Van Assche, Himanshu Madhani, Quinn Tran,
Martin Wilck, Daniel Wagner, Roman Bolshakov
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,
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-12 21:08 Bart Van Assche
@ 2020-01-13 10:02 ` Daniel Wagner
2020-01-13 15:29 ` Ewan D. Milne
2020-01-14 10:09 ` Roman Bolshakov
2 siblings, 0 replies; 11+ messages in thread
From: Daniel Wagner @ 2020-01-13 10:02 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Himanshu Madhani, Quinn Tran, Martin Wilck, Roman Bolshakov
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-12 21:08 Bart Van Assche
2020-01-13 10:02 ` Daniel Wagner
@ 2020-01-13 15:29 ` Ewan D. Milne
2020-01-13 16:11 ` Bart Van Assche
2020-01-14 10:09 ` Roman Bolshakov
2 siblings, 1 reply; 11+ messages in thread
From: Ewan D. Milne @ 2020-01-13 15:29 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Himanshu Madhani, Quinn Tran, Martin Wilck,
Daniel Wagner, Roman Bolshakov
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>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-13 15:29 ` Ewan D. Milne
@ 2020-01-13 16:11 ` Bart Van Assche
2020-01-14 18:13 ` Ewan D. Milne
0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2020-01-13 16:11 UTC (permalink / raw)
To: Ewan D. Milne, Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Himanshu Madhani, Quinn Tran, Martin Wilck,
Daniel Wagner, Roman Bolshakov
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-13 16:11 ` Bart Van Assche
@ 2020-01-14 18:13 ` Ewan D. Milne
2020-01-15 2:56 ` Bart Van Assche
0 siblings, 1 reply; 11+ messages in thread
From: Ewan D. Milne @ 2020-01-14 18:13 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Himanshu Madhani, Quinn Tran, Martin Wilck,
Daniel Wagner, Roman Bolshakov
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-14 18:13 ` Ewan D. Milne
@ 2020-01-15 2:56 ` Bart Van Assche
2020-01-15 16:08 ` Ewan D. Milne
0 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2020-01-15 2:56 UTC (permalink / raw)
To: Ewan D. Milne, Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Himanshu Madhani, Quinn Tran, Martin Wilck,
Daniel Wagner, Roman Bolshakov
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-15 2:56 ` Bart Van Assche
@ 2020-01-15 16:08 ` Ewan D. Milne
0 siblings, 0 replies; 11+ messages in thread
From: Ewan D. Milne @ 2020-01-15 16:08 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen, James E . J . Bottomley
Cc: linux-scsi, Himanshu Madhani, Quinn Tran, Martin Wilck,
Daniel Wagner, Roman Bolshakov
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path
2020-01-12 21:08 Bart Van Assche
2020-01-13 10:02 ` Daniel Wagner
2020-01-13 15:29 ` Ewan D. Milne
@ 2020-01-14 10:09 ` Roman Bolshakov
2 siblings, 0 replies; 11+ messages in thread
From: Roman Bolshakov @ 2020-01-14 10:09 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, linux-scsi,
Himanshu Madhani, Quinn Tran, Martin Wilck, Daniel Wagner,
Colin Ian King
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-01-20 23:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-18 4:20 [PATCH] qla2xxx: Fix a NULL pointer dereference in an error path Bart Van Assche
2020-01-20 8:58 ` Daniel Wagner
2020-01-20 23:38 ` Martin K. Petersen
-- strict thread matches above, loose matches on Subject: below --
2020-01-12 21:08 Bart Van Assche
2020-01-13 10:02 ` Daniel Wagner
2020-01-13 15:29 ` Ewan D. Milne
2020-01-13 16:11 ` Bart Van Assche
2020-01-14 18:13 ` Ewan D. Milne
2020-01-15 2:56 ` Bart Van Assche
2020-01-15 16:08 ` Ewan D. Milne
2020-01-14 10:09 ` Roman Bolshakov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.