Messages in this thread | | | Subject | Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap() | From | Jason Wang <> | Date | Thu, 15 Jul 2021 10:20:40 +0800 |
| |
在 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 >
| |