Re: [ovs-dev] [PATCH net-next] net: openvswitch: allow providing upcall pid for the 'execute' command

[Ilya Maximets]

On 7/3/25 1:04 PM, Flavio Leitner wrote:
> On Thu, 3 Jul 2025 10:38:49 +0200
> Ilya Maximets <i.maximets@xxxxxxx> wrote:
> 
>> On 7/3/25 1:08 AM, Flavio Leitner wrote:
>>>>>> @@ -651,6 +654,10 @@ static int ovs_packet_cmd_execute(struct sk_buff
>>>>>> *skb, struct genl_info *info) !!(hash & OVS_PACKET_HASH_L4_BIT));
>>>>>>  	}
>>>>>>  
>>>>>> +	if (a[OVS_PACKET_ATTR_UPCALL_PID])
>>>>>> +		upcall_pid =
>>>>>> nla_get_u32(a[OVS_PACKET_ATTR_UPCALL_PID]);
>>>>>> +	OVS_CB(packet)->upcall_pid = upcall_pid;  
>>>
>>> Since this is coming from userspace, does it make sense to check if the
>>> upcall_pid is one of the pids in the dp->upcall_portids array?  
>>
>> Not really.  IMO, this would be an unnecessary artificial restriction.
>> We're not concerned about security here since OVS_PACKET_CMD_EXECUTE
>> requires the same privileges as the OVS_DP_CMD_NEW or the
>> OVS_DP_CMD_SET.
> 
> What if the userspace is buggy or compromised?
> It seems netlink API will return -ECONNREFUSED and the upcall is dropped.
> Therefore, we would be okay either way, correct?

If the userspace is compromised, it can overwrite the upcall_portids
and do many other things, since the userspace application here has a
CAP_NET_ADMIN.  And if it's buggy, then the packet will be just dropped
on validation or on the upcall, there isn't much difference.

It's a responsibility of the userspace application to make sure these
sockets exist before passing PIDs into the kernel.  From the kernel's
perspective dropping the upcall is completely fine.  So, yes, we should
be OK.

Best regards, Ilya Maximets.