[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page com
From: |
Peter Maydell |
Subject: |
Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page command |
Date: |
Tue, 29 Sep 2020 14:11:03 +0100 |
On Mon, 6 Jul 2020 at 07:15, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@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
- Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page command,
Peter Maydell <=