netdev - Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds

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] [day] [month] [year] [list]

Message-ID: <fc06afd5-0e2d-c3ae-c118-3292e16db186@redhat.com>
Date:   Wed, 25 Sep 2019 11:59:56 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "wangxu (AE)" <wangxu72@...wei.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of
 vhost_exceeds_weight()


On 2019/9/23 下午5:12, wangxu (AE) wrote:
> Hi Michael
>
> 	Thanks for your fast reply.
>
> 	As the following code, the 2nd branch of iov_iter_advance() does not check if i->count < size, when this happens, i->count -= size may cause len exceed INT_MAX, and then total_len exceed INT_MAX.
>
> 	handle_tx_copy() ->
> 		get_tx_bufs(..., &len, ...) ->
> 			init_iov_iter() ->
> 				iov_iter_advance(iter, ...) 	// has 3 branches:
> 					pipe_advance() 	 	// has checked the size: if (unlikely(i->count < size)) size = i->count;
> 					iov_iter_is_discard() ... 	// no check.


Yes, but I don't think we use ITER_DISCARD.

Thanks


> 					iterate_and_advance() 	//has checked: if (unlikely(i->count < n)) n = i->count;
> 				return iov_iter_count(iter);
>
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@...hat.com]
> Sent: Monday, September 23, 2019 4:07 PM
> To: wangxu (AE) <wangxu72@...wei.com>
> Cc: jasowang@...hat.com; kvm@...r.kernel.org; virtualization@...ts.linux-foundation.org; netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight()
>
> On Mon, Sep 23, 2019 at 03:46:41PM +0800, wangxu wrote:
>> From: Wang Xu <wangxu72@...wei.com>
>>
>> Caller of vhost_exceeds_weight(..., total_len) in drivers/vhost/net.c
>> usually pass size_t total_len, which may be affected by rx/tx package.
>>
>> Signed-off-by: Wang Xu <wangxu72@...wei.com>
>
> Puts a bit more pressure on the register file ...
> why do we care? Is there some way that it can exceed INT_MAX?
>
>> ---
>>   drivers/vhost/vhost.c | 4 ++--
>>   drivers/vhost/vhost.h | 7 ++++---
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index
>> 36ca2cf..159223a 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -412,7 +412,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev
>> *dev)  }
>>   
>>   bool vhost_exceeds_weight(struct vhost_virtqueue *vq,
>> -			  int pkts, int total_len)
>> +			  int pkts, size_t total_len)
>>   {
>>   	struct vhost_dev *dev = vq->dev;
>>   
>> @@ -454,7 +454,7 @@ static size_t vhost_get_desc_size(struct
>> vhost_virtqueue *vq,
>>   
>>   void vhost_dev_init(struct vhost_dev *dev,
>>   		    struct vhost_virtqueue **vqs, int nvqs,
>> -		    int iov_limit, int weight, int byte_weight)
>> +		    int iov_limit, int weight, size_t byte_weight)
>>   {
>>   	struct vhost_virtqueue *vq;
>>   	int i;
>> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index
>> e9ed272..8d80389d 100644
>> --- a/drivers/vhost/vhost.h
>> +++ b/drivers/vhost/vhost.h
>> @@ -172,12 +172,13 @@ struct vhost_dev {
>>   	wait_queue_head_t wait;
>>   	int iov_limit;
>>   	int weight;
>> -	int byte_weight;
>> +	size_t byte_weight;
>>   };
>>   
>
> This just costs extra memory, and value is never large, so I don't think this matters.
>
>> -bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int
>> total_len);
>> +bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts,
>> +			  size_t total_len);
>>   void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs,
>> -		    int nvqs, int iov_limit, int weight, int byte_weight);
>> +		    int nvqs, int iov_limit, int weight, size_t byte_weight);
>>   long vhost_dev_set_owner(struct vhost_dev *dev);  bool
>> vhost_dev_has_owner(struct vhost_dev *dev);  long
>> vhost_dev_check_owner(struct vhost_dev *);
>> --
>> 1.8.5.6