On Tue, Sep 29, 2020 at 11:46:00PM +0200, Klaus Jensen wrote: > On Sep 29 14:11, Peter Maydell wrote: > > > +static uint16_t nvme_fw_log_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t > > > buf_len, > > > + uint64_t off, NvmeRequest *req) > > > +{ > > > + uint32_t trans_len; > > > + uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1); > > > + uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2); > > > + NvmeFwSlotInfoLog fw_log = { > > > + .afi = 0x1, > > > + }; > > > + > > > + strpadcpy((char *)&fw_log.frs1, sizeof(fw_log.frs1), "1.0", ' '); > > > + > > > + if (off > sizeof(fw_log)) { > > > + return NVME_INVALID_FIELD | NVME_DNR; > > > + } > > > + > > > + trans_len = MIN(sizeof(fw_log) - off, buf_len); > > > + > > > + return nvme_dma_read_prp(n, (uint8_t *) &fw_log + off, trans_len, > > > prp1, > > > + prp2); > > > > Coverity warns about the same structure here (CID 1432411). > > > > thanks > > -- PMM > > Hi Peter, > > Thanks. This is somewhere in the middle of a bunch of patches I got > merged I think, commit 94a7897c41db? I just requested Coverity access. > > What happens is that nvme_dma_read_prp will call into nvme_map_prp which > wont map anything because len is 0. This will cause the statically > allocated QEMUSGList and QEMUIOVector in the request to be > uninitialized. Returning from nvme_map_prp, nvme_dma_read_prp will > notice that req->qsg.nsg is zero so it will default to the iov and move > into qemu_iovec_{to,from}_buf(&req->iov, ...). In there we actually pass > the NULL struct iovec, but since there is a __builtin_constant_p(bytes) > condition at the end of it all, we never follow it. > > Not "serious" I think, but definitely not good. We will of course fix > this up. > > @keith, do you agree with my analysis?
Yeah, looks safe as-is, but we're missing out on returning the spec required 'Invalid Field'.