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

[sunichi <sunyiqixm@gmail.com>]

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.