Re: [PATCH v3 net] openvswitch: limit vport upcall portids to the number of CPUs

[Ilya Maximets <i.maximets@ovn.org>]

On 4/13/26 5:55 AM, Weiming Shi wrote:
> The vport netlink reply helpers allocate a fixed-size skb with
> nlmsg_new(NLMSG_DEFAULT_SIZE, ...) but serialize the full upcall PID
> array via ovs_vport_get_upcall_portids().  Since
> ovs_vport_set_upcall_portids() accepts any non-zero multiple of
> sizeof(u32) with no upper bound, a CAP_NET_ADMIN user can install a PID
> array large enough to overflow the reply buffer, causing nla_put() to
> fail with -EMSGSIZE and hitting BUG_ON(err < 0).  On systems with
> unprivileged user namespaces enabled (e.g., Ubuntu default), this is
> reachable via unshare -Urn since OVS vport mutation operations use
> GENL_UNS_ADMIN_PERM.
> 
>   kernel BUG at net/openvswitch/datapath.c:2414!
>   Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>   CPU: 1 UID: 0 PID: 65 Comm: poc Not tainted 7.0.0-rc7-00195-geb216e422044 #1
>   RIP: 0010:ovs_vport_cmd_set+0x34c/0x400
>   Call Trace:
>    <TASK>
>    genl_family_rcv_msg_doit (net/netlink/genetlink.c:1116)
>    genl_rcv_msg (net/netlink/genetlink.c:1194)
>    netlink_rcv_skb (net/netlink/af_netlink.c:2550)
>    genl_rcv (net/netlink/genetlink.c:1219)
>    netlink_unicast (net/netlink/af_netlink.c:1344)
>    netlink_sendmsg (net/netlink/af_netlink.c:1894)
>    __sys_sendto (net/socket.c:2206)
>    __x64_sys_sendto (net/socket.c:2209)
>    do_syscall_64 (arch/x86/entry/syscall_64.c:63)
>    entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
>    </TASK>
>   Kernel panic - not syncing: Fatal exception
> 
> Reject attempts to set more PIDs than num_possible_cpus() in

Any reason not to use nr_cpu_ids?  If not, then its better to switch to
that to be consistent with the per-cpu dispatch configuration.

> ovs_vport_set_upcall_portids(), and pre-compute the worst-case reply
> size in ovs_vport_cmd_msg_size() based on that bound, similar to the
> existing ovs_dp_cmd_msg_size().
> 
> Fixes: 5cd667b0a456 ("openvswitch: Allow each vport to have an array of 'port_id's.")
> Reported-by: Xiang Mei <xmei5@asu.edu>
> Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> ---
> v3:
>  - Cap PID array at num_possible_cpus() in ovs_vport_set_upcall_portids().
>  - Add ovs_vport_cmd_msg_size() for worst-case reply allocation.
>  - Keep BUG_ON()s, fix Fixes tag.
> v2:
>  - Dynamically size reply skb instead of using fixed NLMSG_DEFAULT_SIZE.
>  - Drop WARN_ON_ONCE; use plain error returns instead.
> 
>  net/openvswitch/datapath.c | 23 +++++++++++++++++++++--
>  net/openvswitch/vport.c    |  3 +++
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index e209099218b4..4049bfa1c4df 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -2184,9 +2184,28 @@ static int ovs_vport_cmd_fill_info(struct vport *vport, struct sk_buff *skb,
>  	return err;
>  }
>  
> +static size_t ovs_vport_cmd_msg_size(void)
> +{
> +	size_t msgsize = NLMSG_ALIGN(sizeof(struct ovs_header));
> +
> +	msgsize += nla_total_size(sizeof(u32)); /* OVS_VPORT_ATTR_PORT_NO */
> +	msgsize += nla_total_size(sizeof(u32)); /* OVS_VPORT_ATTR_TYPE */
> +	msgsize += nla_total_size(IFNAMSIZ);
> +	msgsize += nla_total_size(sizeof(u32)); /* OVS_VPORT_ATTR_IFINDEX */
> +	msgsize += nla_total_size(sizeof(s32)); /* OVS_VPORT_ATTR_NETNSID */
> +	msgsize += nla_total_size_64bit(sizeof(struct ovs_vport_stats));
> +	msgsize += nla_total_size(nla_total_size_64bit(sizeof(u64)) +
> +				  nla_total_size_64bit(sizeof(u64)));
> +	msgsize += nla_total_size(num_possible_cpus() * sizeof(u32));
> +	msgsize += nla_total_size(nla_total_size(sizeof(u16)) +
> +				  nla_total_size(nla_total_size(0)));

Please, add comments about which attributes are included for each line where
it is not obvious.  Plain u16 or u64, for example, are not obvious.  Put them
on separate lines when they do not fit.  E.g.:

	/* OVS_VPORT_ATTR_OPTIONS(OVS_TUNNEL_ATTR_DST_PORT +
	 *                        OVS_TUNNEL_ATTR_EXTENSION(OVS_VXLAN_EXT_GBP))
	 */
	msgsize += nla_total_size(nla_total_size(sizeof(u16)) +
				  nla_total_size(nla_total_size(0)));

Best regards, Ilya Maximets.