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

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

[Weiming Shi]

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
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)));
+
+	return msgsize;
+}
+
 static struct sk_buff *ovs_vport_cmd_alloc_info(void)
 {
-	return nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	return genlmsg_new(ovs_vport_cmd_msg_size(), GFP_KERNEL);
 }
 
 /* Called with ovs_mutex, only via ovs_dp_notify_wq(). */
@@ -2196,7 +2215,7 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *vport, struct net *net,
 	struct sk_buff *skb;
 	int retval;
 
-	skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	skb = ovs_vport_cmd_alloc_info();
 	if (!skb)
 		return ERR_PTR(-ENOMEM);
 
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 23f629e94a36..ccd43bc47bc6 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -406,6 +406,9 @@ int ovs_vport_set_upcall_portids(struct vport *vport, const struct nlattr *ids)
 	if (!nla_len(ids) || nla_len(ids) % sizeof(u32))
 		return -EINVAL;
 
+	if (nla_len(ids) / sizeof(u32) > num_possible_cpus())
+		return -EINVAL;
+
 	old = ovsl_dereference(vport->upcall_portids);
 
 	vport_portids = kmalloc(sizeof(*vport_portids) + nla_len(ids),
-- 
2.43.0

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

[Ilya Maximets]

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.