| Message ID | 20200614223921.5851-8-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qla2xxx patches for kernel v5.9 | expand |
Hi Bart, On Sun, Jun 14, 2020 at 03:39:19PM -0700, Bart Van Assche wrote: > @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha) > } > > if (rval == QLA_SUCCESS) > - qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]); > + qla2xxx_copy_queues(ha, (char *)fw + > + offsetof(typeof(*fw), risc_ram) + cnt); This looks pretty ugly to me. Any chance to write this in a way it's understandable by humans and coverity is not annoyed? Do I understand it correctly, it's valid to read after the end of risc_ram? Thanks, Daniel
On 2020-06-23 01:33, Daniel Wagner wrote: > On Sun, Jun 14, 2020 at 03:39:19PM -0700, Bart Van Assche wrote: >> @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha) >> } >> >> if (rval == QLA_SUCCESS) >> - qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]); >> + qla2xxx_copy_queues(ha, (char *)fw + >> + offsetof(typeof(*fw), risc_ram) + cnt); > > This looks pretty ugly to me. Any chance to write this in a way it's > understandable by humans and coverity is not annoyed? > > Do I understand it correctly, it's valid to read after the end of risc_ram? Doesn't the function qla2xxx_copy_queues() write to the pointer passed as the second argument instead of reading? A possible alternative can be found below. The only reason I can think of why this works is because the qla2100_fw_dump structure is occurs in a union and because there is probably a larger structure present in the same union. I would like to specify a size for the queue_dump[] array but I am not sure where to get that information from. Subject: [PATCH] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump() 'cnt' can exceed the size of the risc_ram[] array. Prevent that Coverity complains by rewriting an address calculation expression. This patch fixes the following Coverity complaint: CID 337803 (#1 of 1): Out-of-bounds read (OVERRUN) 109. overrun-local: Overrunning array of 122880 bytes at byte offset 122880 by dereferencing pointer &fw->risc_ram[cnt]. --- drivers/scsi/qla2xxx/qla_dbg.c | 2 +- drivers/scsi/qla2xxx/qla_dbg.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c index 19005710f7f6..41493bd53fc0 100644 --- a/drivers/scsi/qla2xxx/qla_dbg.c +++ b/drivers/scsi/qla2xxx/qla_dbg.c @@ -1063,7 +1063,7 @@ qla2100_fw_dump(scsi_qla_host_t *vha) } if (rval == QLA_SUCCESS) - qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]); + qla2xxx_copy_queues(ha, &fw->queue_dump[0]); qla2xxx_dump_post_process(base_vha, rval); } diff --git a/drivers/scsi/qla2xxx/qla_dbg.h b/drivers/scsi/qla2xxx/qla_dbg.h index 54ed020e6f75..91eb6901815c 100644 --- a/drivers/scsi/qla2xxx/qla_dbg.h +++ b/drivers/scsi/qla2xxx/qla_dbg.h @@ -53,6 +53,7 @@ struct qla2100_fw_dump { __be16 fpm_b0_reg[64]; __be16 fpm_b1_reg[64]; __be16 risc_ram[0xf000]; + u8 queue_dump[]; }; struct qla24xx_fw_dump {
Hi Bart, On Sun, Jun 28, 2020 at 03:31:37PM -0700, Bart Van Assche wrote: > On 2020-06-23 01:33, Daniel Wagner wrote: > > On Sun, Jun 14, 2020 at 03:39:19PM -0700, Bart Van Assche wrote: > >> @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha) > >> } > >> > >> if (rval == QLA_SUCCESS) > >> - qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]); > >> + qla2xxx_copy_queues(ha, (char *)fw + > >> + offsetof(typeof(*fw), risc_ram) + cnt); > > > > This looks pretty ugly to me. Any chance to write this in a way it's > > understandable by humans and coverity is not annoyed? > > > > Do I understand it correctly, it's valid to read after the end of risc_ram? > > Doesn't the function qla2xxx_copy_queues() write to the pointer passed > as the second argument instead of reading? Yes, it seems to copy the request queue and the response queue to the pointer. Funny, it returns a pointer offset which only uses the size of the response queue which is used py qla2xxx_copy_eft. Looking at I would swear qla2xxx_copy_eft overwrites data from qla2xxx_copy_queues. > A possible alternative can be found below. The only reason I can think > of why this works is because the qla2100_fw_dump structure is occurs in > a union and because there is probably a larger structure present in the > same union. I would like to specify a size for the queue_dump[] array > but I am not sure where to get that information from. Yes, this would be indeed a good thing. > Subject: [PATCH] qla2xxx: Fix a Coverity complaint in qla2100_fw_dump() > > 'cnt' can exceed the size of the risc_ram[] array. Prevent that Coverity > complains by rewriting an address calculation expression. This patch > fixes the following Coverity complaint: > > CID 337803 (#1 of 1): Out-of-bounds read (OVERRUN) > 109. overrun-local: Overrunning array of 122880 bytes at byte offset > 122880 by dereferencing pointer &fw->risc_ram[cnt]. Much better, now I get it :) Reviewed-by: Daniel Wagner <dwagner@suse.de> Thanks, Daniel
diff --git a/drivers/scsi/qla2xxx/qla_dbg.c b/drivers/scsi/qla2xxx/qla_dbg.c index 2ed0b849fbfe..5e873b70e843 100644 --- a/drivers/scsi/qla2xxx/qla_dbg.c +++ b/drivers/scsi/qla2xxx/qla_dbg.c @@ -1063,7 +1063,8 @@ qla2100_fw_dump(scsi_qla_host_t *vha) } if (rval == QLA_SUCCESS) - qla2xxx_copy_queues(ha, &fw->risc_ram[cnt]); + qla2xxx_copy_queues(ha, (char *)fw + + offsetof(typeof(*fw), risc_ram) + cnt); qla2xxx_dump_post_process(base_vha, rval); }
'cnt' can exceed the size of the risc_ram[] array. Prevent that Coverity complains by rewriting an address calculation expression. This patch fixes the following Coverity complaint: CID 337803 (#1 of 1): Out-of-bounds read (OVERRUN) 109. overrun-local: Overrunning array of 122880 bytes at byte offset 122880 by dereferencing pointer &fw->risc_ram[cnt]. Cc: Nilesh Javali <njavali@marvell.com> Cc: Quinn Tran <qutran@marvell.com> Cc: Himanshu Madhani <himanshu.madhani@oracle.com> Cc: Daniel Wagner <dwagner@suse.de> Cc: Martin Wilck <mwilck@suse.com> Cc: Roman Bolshakov <r.bolshakov@yadro.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/qla2xxx/qla_dbg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)