On Mar 25 12:45, Maxim Levitsky wrote:
> On Mon, 2020-03-16 at 07:29 -0700, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jen...@samsung.com>
> >
> > Add nvme_map_addr, nvme_map_addr_cmb and nvme_addr_to_cmb helpers and
> > use them in nvme_map_prp.
> >
> > This fixes a bug where in the case of a CMB transfer, the device would
> > map to the buffer with a wrong length.
> >
> > Fixes: b2b2b67a00574 ("nvme: Add support for Read Data and Write Data in
> > CMBs.")
> > Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
> > ---
> > hw/block/nvme.c | 97 +++++++++++++++++++++++++++++++++++--------
> > hw/block/trace-events | 1 +
> > 2 files changed, 81 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 08267e847671..187c816eb6ad 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -153,29 +158,79 @@ static void nvme_irq_deassert(NvmeCtrl *n, NvmeCQueue
> > *cq)
> > }
> > }
> >
> > +static uint16_t nvme_map_addr_cmb(NvmeCtrl *n, QEMUIOVector *iov, hwaddr
> > addr,
> > + size_t len)
> > +{
> > + if (!nvme_addr_is_cmb(n, addr) || !nvme_addr_is_cmb(n, addr + len)) {
> > + return NVME_DATA_TRAS_ERROR;
> > + }
>
> I just noticed that
> in theory (not that it really matters) but addr+len refers to the byte which
> is already
> not the part of the transfer.
>
Oh. Good catch - and I think that it does matter? Can't we end up
rejecting a valid access? Anyway, I fixed it with a '- 1'.
>
> > +
> > + qemu_iovec_add(iov, nvme_addr_to_cmb(n, addr), len);
> Also intersting is we can add 0 sized iovec.
>
I added a check on len. This also makes sure the above '- 1' fix doesn't
cause an 'addr + 0 - 1' to be done.
