Re: [PATCH v3 07/18] hw/block/nvme: add support for the get log page command

[Klaus Jensen]

On Sep 29 15:34, Keith Busch wrote:
> 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'.

I can't see where it says that we should do that? Invalid Field in
Command if offset is *greater* than the size of the log page.

Some dynamic log pages have side-effects of being read, so while this is
a super wierd way of specifying that we want nothing returned, I think
it is valid?

signature.asc
Description: PGP signature