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

Message-ID: <20250622160221.GH71935@horms.kernel.org>
Date: Sun, 22 Jun 2025 17:02:21 +0100
From: Simon Horman <horms@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
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 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.

> +			return -EFAULT;
> +		return 0;

...