netdev - Re: [PATCH v5 net-next 4/9] vhost-net: allow configuring extended features

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: <61d10c9b-4f1b-46f6-9a52-1de9aa193a7b@redhat.com>
Date: Mon, 23 Jun 2025 12:58:36 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, Willem de Bruijn
 <willemdebruijn.kernel@...il.com>, Jason Wang <jasowang@...hat.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>, Yuri Benditovich <yuri.benditovich@...nix.com>,
 Akihiko Odaki <akihiko.odaki@...nix.com>, Jonathan Corbet <corbet@....net>,
 kvm@...r.kernel.org
Subject: Re: [PATCH v5 net-next 4/9] vhost-net: allow configuring extended
 features

On 6/22/25 6:02 PM, Simon Horman wrote:
> On Fri, Jun 20, 2025 at 07:39:48PM +0200, Paolo Abeni wrote:
>> Use the extended feature type for 'acked_features' and implement
>> two new ioctls operation allowing the user-space to set/query an
>> unbounded amount of features.
>>
>> The actual number of processed features is limited by VIRTIO_FEATURES_MAX
>> and attempts to set features above such limit fail with
>> EOPNOTSUPP.
>>
>> Note that: the legacy ioctls implicitly truncate the negotiated
>> features to the lower 64 bits range and the 'acked_backend_features'
>> field don't need conversion, as the only negotiated feature there
>> is in the low 64 bit range.
>>
>> Acked-by: Jason Wang <jasowang@...hat.com>
>> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> 
> ...
> 
>> +	case VHOST_GET_FEATURES_ARRAY:
>> +		if (get_user(count, featurep))
>> +			return -EFAULT;
>> +
>> +		/* Copy the net features, up to the user-provided buffer size */
>> +		argp += sizeof(u64);
>> +		copied = min(count, VIRTIO_FEATURES_DWORDS);
>> +		if (copy_to_user(argp, vhost_net_features,
>> +				 copied * sizeof(u64)))
>> +			return -EFAULT;
>> +
>> +		/* Zero the trailing space provided by user-space, if any */
>> +		if (clear_user(argp, (count - copied) * sizeof(u64)))
> 
> Hi Paolo,
> 
> Smatch warns to "check for integer overflow 'count'" on the line above.
> 
> Perhaps it is wrong. Or my analyais is. But it seems to me that an overflow
> could occur if count is very large, say such that (count - copied) is more
> than 2^64 / 8.  As then (count - copied) * sizeof(u64) would overflow 64
> bits.
> 
> By the same reasoning this could overflow 32 bits on systems where an
> unsigned long, type type of the 2nd parameter of clear_user, is 32 bits.

I think you and smatch are right. I'll use size_mul() in the next
iteration. I'll wait a little more before posting it to possibly allow
for more reviews.

Thanks,

Paolo