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...
