On Sep 29 14:11, Peter Maydell wrote:
> 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
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?
signature.asc
Description: PGP signature
