Virtualization - Jul 2021 - [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()

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

Thanks

> 	    msg->iova + msg->size - 1 > v->range.last)
>
> But writing integer overflow check correctly is notoriously difficult.
> Do you think you could send a fix for that which is separate from the
> patcheset?  We'd want to backport it to stable.
>
> regards,
> dan carpenter
>

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