[PATCH] net/openvswitch: fix trigger-able BUG_ON after ovs_vport_cmd_fill_info

[PATCH] net/openvswitch: fix trigger-able BUG_ON after ovs_vport_cmd_fill_info

[sunichi]

ovs_vport_set_upcall_portids() does not validate the length of the
user-supplied OVS_VPORT_ATTR_UPCALL_PID netlink attribute. A
sufficiently large portid list can overflow the reply skb allocated
with NLMSG_DEFAULT_SIZE in causing ovs_vport_cmd_fill_info()
to return -EMSGSIZE and triggering the unconditional BUG_ON(),
which panics the kernel on most distributions.

Any local user with CAP_NET_ADMIN (or an equivalent unprivileged
namespace capability where applicable) can exploit this to perform a
denial-of-service against the host.

Replace BUG_ON with WARN_ON_ONCE to prevent kernel panic.

Signed-off-by: sunichi <sunyiqixm@gmail.com>
---
 net/openvswitch/datapath.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index e209099218b4..50c2945081a1 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -2202,7 +2202,8 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *vport, struct net *net,
 
 	retval = ovs_vport_cmd_fill_info(vport, skb, net, portid, seq, 0, cmd,
 					 GFP_KERNEL);
-	BUG_ON(retval < 0);
+	if (WARN_ON_ONCE(retval < 0))
+		return ERR_PTR(-EMSGSIZE);
 
 	return skb;
 }
@@ -2358,7 +2359,9 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
 	else
 		netdev_set_rx_headroom(vport->dev, dp->max_headroom);
 
-	BUG_ON(err < 0);
+	if (WARN_ON_ONCE(err < 0))
+		goto exit_unlock_free;
+
 	ovs_unlock();
 
 	ovs_notify(&dp_vport_genl_family, reply, info);
@@ -2411,7 +2414,8 @@ static int ovs_vport_cmd_set(struct sk_buff *skb, struct genl_info *info)
 	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
 				      info->snd_portid, info->snd_seq, 0,
 				      OVS_VPORT_CMD_SET, GFP_KERNEL);
-	BUG_ON(err < 0);
+	if (WARN_ON_ONCE(err < 0))
+		goto exit_unlock_free;
 
 	ovs_unlock();
 	ovs_notify(&dp_vport_genl_family, reply, info);
@@ -2451,7 +2455,8 @@ static int ovs_vport_cmd_del(struct sk_buff *skb, struct genl_info *info)
 	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
 				      info->snd_portid, info->snd_seq, 0,
 				      OVS_VPORT_CMD_DEL, GFP_KERNEL);
-	BUG_ON(err < 0);
+	if (WARN_ON_ONCE(err < 0))
+		goto exit_unlock_free;
 
 	/* the vport deletion may trigger dp headroom update */
 	dp = vport->dp;
@@ -2498,7 +2503,9 @@ static int ovs_vport_cmd_get(struct sk_buff *skb, struct genl_info *info)
 	err = ovs_vport_cmd_fill_info(vport, reply, genl_info_net(info),
 				      info->snd_portid, info->snd_seq, 0,
 				      OVS_VPORT_CMD_GET, GFP_ATOMIC);
-	BUG_ON(err < 0);
+	if (WARN_ON_ONCE(err < 0))
+		goto exit_unlock_free;
+
 	rcu_read_unlock();
 
 	return genlmsg_reply(reply, info);
-- 
2.34.1

Re: [PATCH] net/openvswitch: fix trigger-able BUG_ON after ovs_vport_cmd_fill_info

[Ilya Maximets]

On 3/23/26 8:14 AM, sunichi wrote:
> ovs_vport_set_upcall_portids() does not validate the length of the
> user-supplied OVS_VPORT_ATTR_UPCALL_PID netlink attribute. A
> sufficiently large portid list can overflow the reply skb allocated
> with NLMSG_DEFAULT_SIZE in causing ovs_vport_cmd_fill_info()
> to return -EMSGSIZE and triggering the unconditional BUG_ON(),
> which panics the kernel on most distributions.
> 
> Any local user with CAP_NET_ADMIN (or an equivalent unprivileged
> namespace capability where applicable) can exploit this to perform a
> denial-of-service against the host.
> 
> Replace BUG_ON with WARN_ON_ONCE to prevent kernel panic.
> 
> Signed-off-by: sunichi <sunyiqixm@gmail.com>
> ---
>  net/openvswitch/datapath.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index e209099218b4..50c2945081a1 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -2202,7 +2202,8 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *vport, struct net *net,
>  
>  	retval = ovs_vport_cmd_fill_info(vport, skb, net, portid, seq, 0, cmd,
>  					 GFP_KERNEL);
> -	BUG_ON(retval < 0);
> +	if (WARN_ON_ONCE(retval < 0))
> +		return ERR_PTR(-EMSGSIZE);

Hi, sunichi.  Thanks for the patch!  Though I don't think this is the right
solution.  Instead of just failing the request, we should allocate appropriate
amount of memory for it instead.  The fact that the array is sort of unbounded
today is also a problem.

So, what I'd suggest is, let's limit the number of upcall pids for a vport with
the number of CPUs as it is done for the upcall_pids array on the datapath level.
This will give us some reasonable upper value as there is no point for the
application to have more handlers than there are CPUs, and the existing userspace
never does that, so the limit should be safe.  Next we can create a new function
ovs_vport_cmd_msg_size() similar to the existing ovs_dp_cmd_msg_size() that would
calculate and allocate the appropriate message size, so the allocation is always
correct.

P.S.: This patch also needs a Fixes tag and should be targeted for the 'net'
tree, i.e. have [PATCH net] as a subject prefix.  Also, IIRC, kernel requires
a full name in the sign-off tag.

AI review also points out a memory leak in case we just return without freeing
and potentially leaving half- or even fully configured port while returning an
error to the userspace.

Best regards, Ilya Maximets.

Re: Re: [PATCH] net/openvswitch: fix trigger-able BUG_ON after ovs_vport_cmd_fill_info

[sunichi]

On 3/24/26 1:01 PM, Ilya Maximets wrote:
> On 3/23/26 8:14 AM, sunichi wrote:
> > ovs_vport_set_upcall_portids() does not validate the length of the
> > user-supplied OVS_VPORT_ATTR_UPCALL_PID netlink attribute. A
> > sufficiently large portid list can overflow the reply skb allocated
> > with NLMSG_DEFAULT_SIZE in causing ovs_vport_cmd_fill_info()
> > to return -EMSGSIZE and triggering the unconditional BUG_ON(),
> > which panics the kernel on most distributions.
> > 
> > Any local user with CAP_NET_ADMIN (or an equivalent unprivileged
> > namespace capability where applicable) can exploit this to perform a
> > denial-of-service against the host.
> > 
> > Replace BUG_ON with WARN_ON_ONCE to prevent kernel panic.
> > 
> > Signed-off-by: sunichi <sunyiqixm@gmail.com>
> > ---
> >  net/openvswitch/datapath.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> > index e209099218b4..50c2945081a1 100644
> > --- a/net/openvswitch/datapath.c
> > +++ b/net/openvswitch/datapath.c
> > @@ -2202,7 +2202,8 @@ struct sk_buff *ovs_vport_cmd_build_info(struct vport *vport, struct net *net,
> >  
> >  	retval = ovs_vport_cmd_fill_info(vport, skb, net, portid, seq, 0, cmd,
> >  					 GFP_KERNEL);
> > -	BUG_ON(retval < 0);
> > +	if (WARN_ON_ONCE(retval < 0))
> > +		return ERR_PTR(-EMSGSIZE);
> 
> Hi, sunichi.  Thanks for the patch!  Though I don't think this is the right
> solution.  Instead of just failing the request, we should allocate appropriate
> amount of memory for it instead.  The fact that the array is sort of unbounded
> today is also a problem.
> 
> So, what I'd suggest is, let's limit the number of upcall pids for a vport with
> the number of CPUs as it is done for the upcall_pids array on the datapath level.
> This will give us some reasonable upper value as there is no point for the
> application to have more handlers than there are CPUs, and the existing userspace
> never does that, so the limit should be safe.  Next we can create a new function
> ovs_vport_cmd_msg_size() similar to the existing ovs_dp_cmd_msg_size() that would
> calculate and allocate the appropriate message size, so the allocation is always
> correct.

Thanks for the detailed review, Ilya!

Agreed on all points. For v2 patch submit later, would:
1. Add ovs_vport_cmd_msg_size() to calculate the correct allocation size upfront,
eliminating the EMSGSIZE path entirely.
2. Fix the memory leak on the error path.

> P.S.: This patch also needs a Fixes tag and should be targeted for the 'net'
> tree, i.e. have [PATCH net] as a subject prefix.  Also, IIRC, kernel requires
> a full name in the sign-off tag.
> 
> AI review also points out a memory leak in case we just return without freeing
> and potentially leaving half- or even fully configured port while returning an
> error to the userspace.
> 
> Best regards, Ilya Maximets.
> 

Respectfully, sunichi.