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@xxxxxxx> Thanks, Daniel
