On Mon, 6 Jul 2020 at 07:15, Klaus Jensen <i...@irrelevant.dk> wrote: > > From: Klaus Jensen <k.jen...@samsung.com> > > Add support for the Get Log Page command and basic implementations of > the mandatory Error Information, SMART / Health Information and Firmware > Slot Information log pages. > > In violation of the specification, the SMART / Health Information log > page does not persist information over the lifetime of the controller > because the device has no place to store such persistent state. > > Note that the LPA field in the Identify Controller data structure > intentionally has bit 0 cleared because there is no namespace specific > information in the SMART / Health information log page. > > Required for compliance with NVMe revision 1.3d. See NVM Express 1.3d, > Section 5.14 ("Get Log Page command").
Hi; Coverity reports a potential issue in this code (CID 1432413): > +static uint16_t nvme_smart_info(NvmeCtrl *n, NvmeCmd *cmd, uint32_t buf_len, > + uint64_t off, NvmeRequest *req) > +{ > + uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1); > + uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2); > + uint32_t nsid = le32_to_cpu(cmd->nsid); > + > + uint32_t trans_len; > + time_t current_ms; > + uint64_t units_read = 0, units_written = 0; > + uint64_t read_commands = 0, write_commands = 0; > + NvmeSmartLog smart; > + BlockAcctStats *s; > + > + if (nsid && nsid != 0xffffffff) { > + return NVME_INVALID_FIELD | NVME_DNR; > + } > + > + s = blk_get_stats(n->conf.blk); > + > + units_read = s->nr_bytes[BLOCK_ACCT_READ] >> BDRV_SECTOR_BITS; > + units_written = s->nr_bytes[BLOCK_ACCT_WRITE] >> BDRV_SECTOR_BITS; > + read_commands = s->nr_ops[BLOCK_ACCT_READ]; > + write_commands = s->nr_ops[BLOCK_ACCT_WRITE]; > + > + if (off > sizeof(smart)) { > + return NVME_INVALID_FIELD | NVME_DNR; > + } Here we check for off > sizeof(smart), which means that we allow off == sizeof(smart)... > + > + trans_len = MIN(sizeof(smart) - off, buf_len); > + return nvme_dma_read_prp(n, (uint8_t *) &smart + off, trans_len, prp1, > + prp2); ...in which case the pointer we pass to nvme_dma_read_prp() will be off the end of the 'smart' object. Now we are passing 0 as the trans_len, so I *think* this function will not actually read the buffer (Coverity is not smart enough to see this); so I could just close the Coverity issue as a false-positive. But maybe there is a clearer-to-humans as well as clearer-to-Coverity way to write this. What do you think ? > +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