lkml.org 
[lkml]   [2021]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap()
From
Date

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

\
 
 \ /
  Last update: 2021-07-15 04:21    [W:0.348 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site