linux-kernel - Re: [PATCH v9 13/17] vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <db02315d-0ffe-f4a2-da67-5a014060fa4a@redhat.com>
Date:   Wed, 14 Jul 2021 17:41:54 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Xie Yongji <xieyongji@...edance.com>, mst@...hat.com,
        stefanha@...hat.com, sgarzare@...hat.com, parav@...dia.com,
        hch@...radead.org, christian.brauner@...onical.com,
        rdunlap@...radead.org, willy@...radead.org,
        viro@...iv.linux.org.uk, axboe@...nel.dk, bcrl@...ck.org,
        corbet@....net, mika.penttila@...tfour.com, joro@...tes.org,
        gregkh@...uxfoundation.org, zhe.he@...driver.com,
        xiaodong.liu@...el.com, songmuchun@...edance.com,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        iommu@...ts.linux-foundation.org, linux-kernel@...r.kernel.org
Subject: 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 = &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
>
>