On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote:
>
> 在 2021/7/13 下午7:31, Dan Carpenter 写道:
> > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote:
> > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size)
> > > }
> > > }
> > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v,
> > > - struct vhost_iotlb_msg *msg)
> > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v,
> > > + u64 iova, u64 size, u64 uaddr, u32 perm)
> > > {
> > > struct vhost_dev *dev = &v->vdev;
> > > - struct vhost_iotlb *iotlb = dev->iotlb;
> > > struct page **page_list;
> > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *);
> > > unsigned int gup_flags = FOLL_LONGTERM;
> > > unsigned long npages, cur_base, map_pfn, last_pfn = 0;
> > > unsigned long lock_limit, sz2pin, nchunks, i;
> > > - u64 iova = msg->iova;
> > > + u64 start = iova;
> > > long pinned;
> > > int ret = 0;
> > > - if (msg->iova < v->range.first ||
> > > - msg->iova + msg->size - 1 > v->range.last)
> > > - return -EINVAL;
> > This is not related to your patch, but can the "msg->iova + msg->size"
> > addition can have an integer overflow. From looking at the callers it
> > seems like it can. msg comes from:
> > vhost_chr_write_iter()
> > --> dev->msg_handler(dev, &msg);
> > --> vhost_vdpa_process_iotlb_msg()
> > --> vhost_vdpa_process_iotlb_update()
>
>
> Yes.
>
>
> >
> > If I'm thinking of the right thing then these are allowed to overflow to
> > 0 because of the " - 1" but not further than that. I believe the check
> > needs to be something like:
> >
> > if (msg->iova < v->range.first ||
> > msg->iova - 1 > U64_MAX - msg->size ||
>
>
> I guess we don't need - 1 here?
The - 1 is important. The highest address is 0xffffffff. So it goes
start + size = 0 and then start + size - 1 == 0xffffffff.
I guess we could move the - 1 to the other side?
msg->iova > U64_MAX - msg->size + 1 ||
regards,
dan carpenter
