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

[Paolo Abeni <pabeni@redhat.com>]

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@redhat.com>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.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