Re: [PATCH v6 07/42] nvme: refactor nvme_addr_read

[Klaus Birkelund Jensen]

On Mar 31 13:41, Maxim Levitsky wrote:
> On Tue, 2020-03-31 at 07:39 +0200, Klaus Birkelund Jensen wrote:
> > On Mar 25 12:38, Maxim Levitsky wrote:
> > > 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 don't think it does - the check is just moved to nvme_addr_is_cmb:

    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;
    }

We check that `addr` is less than `hi`. Maybe the name is unfortunate...