[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: |
Keith Busch |
Subject: |
Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page command |
Date: |
Tue, 29 Sep 2020 15:34:20 -0700 |
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'.