Message ID | 20210713084656.232-14-xieyongji@bytedance.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Introduce VDUSE - vDPA Device in Userspace | expand |
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
在 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 Tue, Jul 13, 2021 at 7:31 PM Dan Carpenter <dan.carpenter@oracle.com> 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 = &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) > 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
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
在 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 > >
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
在 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 >
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index f60a513dac7c..3e260038beba 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -511,7 +511,7 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep, return r; } -static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v, u64 start, u64 last) +static void vhost_vdpa_pa_unmap(struct vhost_vdpa *v, u64 start, u64 last) { struct vhost_dev *dev = &v->vdev; struct vhost_iotlb *iotlb = dev->iotlb; @@ -533,6 +533,11 @@ static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v, u64 start, u64 last) } } +static void vhost_vdpa_iotlb_unmap(struct vhost_vdpa *v, u64 start, u64 last) +{ + return vhost_vdpa_pa_unmap(v, start, last); +} + static void vhost_vdpa_iotlb_free(struct vhost_vdpa *v) { struct vhost_dev *dev = &v->vdev; @@ -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; - - if (vhost_iotlb_itree_first(iotlb, msg->iova, - msg->iova + msg->size - 1)) - return -EEXIST; - /* Limit the use of memory for bookkeeping */ page_list = (struct page **) __get_free_page(GFP_KERNEL); if (!page_list) return -ENOMEM; - if (msg->perm & VHOST_ACCESS_WO) + if (perm & VHOST_ACCESS_WO) gup_flags |= FOLL_WRITE; - npages = PAGE_ALIGN(msg->size + (iova & ~PAGE_MASK)) >> PAGE_SHIFT; + npages = PAGE_ALIGN(size + (iova & ~PAGE_MASK)) >> PAGE_SHIFT; if (!npages) { ret = -EINVAL; goto free; @@ -657,7 +653,7 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, goto unlock; } - cur_base = msg->uaddr & PAGE_MASK; + cur_base = uaddr & PAGE_MASK; iova &= PAGE_MASK; nchunks = 0; @@ -688,7 +684,7 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, csize = (last_pfn - map_pfn + 1) << PAGE_SHIFT; ret = vhost_vdpa_map(v, iova, csize, map_pfn << PAGE_SHIFT, - msg->perm); + perm); if (ret) { /* * Unpin the pages that are left unmapped @@ -717,7 +713,7 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, /* Pin the rest chunk */ ret = vhost_vdpa_map(v, iova, (last_pfn - map_pfn + 1) << PAGE_SHIFT, - map_pfn << PAGE_SHIFT, msg->perm); + map_pfn << PAGE_SHIFT, perm); out: if (ret) { if (nchunks) { @@ -736,13 +732,32 @@ static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, for (pfn = map_pfn; pfn <= last_pfn; pfn++) unpin_user_page(pfn_to_page(pfn)); } - vhost_vdpa_unmap(v, msg->iova, msg->size); + vhost_vdpa_unmap(v, start, size); } unlock: mmap_read_unlock(dev->mm); free: free_page((unsigned long)page_list); return ret; + +} + +static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, + struct vhost_iotlb_msg *msg) +{ + struct vhost_dev *dev = &v->vdev; + struct vhost_iotlb *iotlb = dev->iotlb; + + if (msg->iova < v->range.first || + msg->iova + msg->size - 1 > v->range.last) + return -EINVAL; + + if (vhost_iotlb_itree_first(iotlb, msg->iova, + msg->iova + msg->size - 1)) + return -EEXIST; + + return vhost_vdpa_pa_map(v, msg->iova, msg->size, msg->uaddr, + msg->perm); } static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev,