Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
在 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 = >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, );
--> 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 0x. So it goes
start + size = 0 and then start + size - 1 == 0x.
Right, so actually
msg->iova = 0xfffe, msg->size=2 is valid.
I believe so, yes. It's inclusive of 0xfffe and 0x.
(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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
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 = >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, );
> > > > --> 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 0x. So it goes
> > start + size = 0 and then start + size - 1 == 0x.
>
>
> Right, so actually
>
> msg->iova = 0xfffe, msg->size=2 is valid.
I believe so, yes. It's inclusive of 0xfffe and 0x.
(Not an expert).
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
在 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 = >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, );
--> 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 0x. So it goes
start + size = 0 and then start + size - 1 == 0x.
Right, so actually
msg->iova = 0xfffe, 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
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 = >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, );
> >--> 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 0x. So it goes
start + size = 0 and then start + size - 1 == 0x.
I guess we could move the - 1 to the other side?
msg->iova > U64_MAX - msg->size + 1 ||
regards,
dan carpenter
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
On Tue, Jul 13, 2021 at 7:31 PM Dan Carpenter wrote:
>
> 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 = >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, );
> --> 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)
>
Make sense.
> 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.
>
OK, I will send a patch to fix it.
Thanks,
Yongji
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [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 = >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, );
--> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
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 = >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, );
--> 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
