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

[Maxim Levitsky]

On Tue, 2020-03-31 at 14:48 +0200, Klaus Birkelund Jensen wrote:
> 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...
> 
> 
Oh, I am just blind! sorry about that.
You are 100% right.

Best regards,
        Maxim Levitsky