[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read
From: |
Maxim Levitsky |
Subject: |
Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read |
Date: |
Tue, 31 Mar 2020 13:41:44 +0300 |
On Tue, 2020-03-31 at 07:39 +0200, Klaus Birkelund Jensen wrote:
> On Mar 25 12:38, Maxim Levitsky wrote:
> > On Mon, 2020-03-16 at 07:28 -0700, Klaus Jensen wrote:
> > > From: Klaus Jensen <address@hidden>
> > >
> > > Pull the controller memory buffer check to its own function. The check
> > > will be used on its own in later patches.
> > >
> > > Signed-off-by: Klaus Jensen <address@hidden>
> > > Acked-by: Keith Busch <address@hidden>
> > > ---
> > > hw/block/nvme.c | 16 ++++++++++++----
> > > 1 file changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index b38d7e548a60..08a83d449de3 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -52,14 +52,22 @@
> > >
> > > static void nvme_process_sq(void *opaque);
> > >
> > > +static inline bool nvme_addr_is_cmb(NvmeCtrl *n, hwaddr addr)
> > > +{
> > > + hwaddr low = n->ctrl_mem.addr;
> > > + hwaddr hi = n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size);
> > > +
> > > + return addr >= low && addr < hi;
> > > +}
> > > +
> > > static void nvme_addr_read(NvmeCtrl *n, hwaddr addr, void *buf, int size)
> > > {
> > > - if (n->cmbsz && addr >= n->ctrl_mem.addr &&
> > > - addr < (n->ctrl_mem.addr +
> > > int128_get64(n->ctrl_mem.size))) {
> > > + if (n->cmbsz && nvme_addr_is_cmb(n, addr)) {
> > > memcpy(buf, (void *)&n->cmbuf[addr - n->ctrl_mem.addr], size);
> > > - } else {
> > > - pci_dma_read(&n->parent_obj, addr, buf, size);
> > > + return;
> > > }
> > > +
> > > + pci_dma_read(&n->parent_obj, addr, buf, size);
> > > }
> > >
> > > static int nvme_check_sqid(NvmeCtrl *n, uint16_t sqid)
> >
> > Note that this patch still contains a bug that it removes the check against
> > the accessed
> > size, which you fix in later patch.
> > I prefer to not add a bug in first place
> > However if you have a reason for this, I won't mind.
> >
>
> So yeah. The resons is that there is actually no bug at this point
> because the controller only supports PRPs. I actually thought there was
> a bug as well and reported it to qemu-security some months ago as a
> potential out of bounds access. I was then schooled by Keith on how PRPs
> work ;) Below is a paraphrased version of Keiths analysis.
>
> The PRPs does not cross page boundaries:
True
>
> trans_len = n->page_size - (prp1 % n->page_size);
>
> The PRPs are always verified to be page aligned:
True
>
> if (unlikely(!prp_ent || prp_ent & (n->page_size - 1))) {
>
> and the transfer length wont go above page size. So, since the beginning
> of the address is within the CMB and considering that the CMB is of an
> MB aligned and sized granularity, then we can never cross outside it
> with PRPs.
I understand now, however the reason I am arguing about this is
that this patch actually _removes_ the size bound check
It was before the patch:
n->cmbsz && addr >= n->ctrl_mem.addr &&
addr < (n->ctrl_mem.addr + int128_get64(n->ctrl_mem.size)
>
> I could add the check at this point (because it *is* needed for when
> SGLs are introduced), but I think it would just be noise and I would
> need to explain why the check is there, but not really needed at this
> point. Instead I'm adding a new patch before the SGL patch that explains
> this.
Best regards,
Maxim Levitsky
- [PATCH v6 02/42] nvme: remove superfluous breaks, (continued)
[PATCH v6 08/42] nvme: add support for the abort command, Klaus Jensen, 2020/03/16
[PATCH v6 10/42] nvme: refactor device realization, Klaus Jensen, 2020/03/16
[PATCH v6 04/42] nvme: bump spec data structures to v1.3, Klaus Jensen, 2020/03/16