Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()

quoted hunk

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()

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 ||
	    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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
在 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
在 2021/7/14 下午4:05, 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.

Right, so actually

msg->iova = 0xfffffffe, msg->size=2 is valid.

Thanks


I guess we could move the - 1 to the other side?

	msg->iova > U64_MAX - msg->size + 1 ||

regards,
dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote:
在 2021/7/14 下午4:05, 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.

Right, so actually

msg->iova = 0xfffffffe, msg->size=2 is valid.
I believe so, yes.  It's inclusive of 0xfffffffe and 0xffffffff.
(Not an expert).

regards,
dan carpenter

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
在 2021/7/14 下午5:57, Dan Carpenter 写道:
On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote:
在 2021/7/14 下午4:05, 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.
Right, so actually

msg->iova = 0xfffffffe, msg->size=2 is valid.
I believe so, yes.  It's inclusive of 0xfffffffe and 0xffffffff.
(Not an expert).

I think so, and we probably need to fix vhost_overflow() as well which did:

static bool vhost_overflow(u64 uaddr, u64 size)
{
         /* Make sure 64 bit math will not overflow. */
         return uaddr > ULONG_MAX || size > ULONG_MAX || uaddr > 
ULONG_MAX - size;
}

Thanks


regards,
dan carpenter
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help