Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex

[Aaron Conole <aconole@redhat.com>]

Jakub Kicinski <kuba@kernel.org> writes:

> Recent changes in net-next (commit 759ab1edb56c ("net: store netdevs
> in an xarray")) refactored the handling of pre-assigned ifindexes
> and let syzbot surface a latent problem in ovs. ovs does not validate
> ifindex, making it possible to create netdev ports with negative
> ifindex values. It's easy to repro with YNL:
>
> $ ./cli.py --spec netlink/specs/ovs_datapath.yaml \
>          --do new \
> 	 --json '{"upcall-pid": 1, "name":"my-dp"}'
> $ ./cli.py --spec netlink/specs/ovs_vport.yaml \
> 	 --do new \
> 	 --json '{"upcall-pid": "00000001", "name": "some-port0", "dp-ifindex":3,"ifindex":4294901760,"type":2}'
>
> $ ip link show
> -65536: some-port0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether 7a:48:21:ad:0b:fb brd ff:ff:ff:ff:ff:ff
> ...
>
> Validate the inputes. Now the second command correctly returns:

s/inputes/inputs/

>
> $ ./cli.py --spec netlink/specs/ovs_vport.yaml \
> 	 --do new \
> 	 --json '{"upcall-pid": "00000001", "name": "some-port0", "dp-ifindex":3,"ifindex":4294901760,"type":2}'
>
> lib.ynl.NlError: Netlink error: Numerical result out of range
> nl_len = 108 (92) nl_flags = 0x300 nl_type = 2
> 	error: -34	extack: {'msg': 'integer out of range', 'unknown': [[type:4 len:36] b'\x0c\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x0c\x00\x03\x00\xff\xff\xff\x7f\x00\x00\x00\x00\x08\x00\x01\x00\x08\x00\x00\x00'], 'bad-attr': '.ifindex'}
>
> Accept 0 since it used to be silently ignored.
>
> Fixes: 54c4ef34c4b6 ("openvswitch: allow specifying ifindex of new interfaces")
> Reported-by: syzbot+7456b5dcf65111553320@syzkaller.appspotmail.com
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: pshelar@ovn.org
> CC: andrey.zhadchenko@virtuozzo.com
> CC: brauner@kernel.org
> CC: dev@openvswitch.org
> ---

Thanks for the quick follow up.  I accidentally broke my system trying
to setup to reproduce the syzbot splat.

The attribute here isn't used by the ovs-vswitchd, so probably why we
never caught an issue before.  I'll think about how to improve the
fuzzing on the ovs module.  At the very least, maybe we can have some
additional checks in the netlink selftest.

I noticed that since I copied the definitions when building
ovs-dpctl.py, I have the same kind of mistake there (using unsigned for
ifindex).  I can submit a follow up to correct that definition.  Also,
we might consider correcting the yaml.

For the main change:

Reviewed-by: Aaron Conole <aconole@redhat.com>

>  net/openvswitch/datapath.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index a6d2a0b1aa21..3d7a91e64c88 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1829,7 +1829,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	parms.port_no = OVSP_LOCAL;
>  	parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
>  	parms.desired_ifindex = a[OVS_DP_ATTR_IFINDEX]
> -		? nla_get_u32(a[OVS_DP_ATTR_IFINDEX]) : 0;
> +		? nla_get_s32(a[OVS_DP_ATTR_IFINDEX]) : 0;
>  
>  	/* So far only local changes have been made, now need the lock. */
>  	ovs_lock();
> @@ -2049,7 +2049,7 @@ static const struct nla_policy datapath_policy[OVS_DP_ATTR_MAX + 1] = {
>  	[OVS_DP_ATTR_USER_FEATURES] = { .type = NLA_U32 },
>  	[OVS_DP_ATTR_MASKS_CACHE_SIZE] =  NLA_POLICY_RANGE(NLA_U32, 0,
>  		PCPU_MIN_UNIT_SIZE / sizeof(struct mask_cache_entry)),
> -	[OVS_DP_ATTR_IFINDEX] = {.type = NLA_U32 },
> +	[OVS_DP_ATTR_IFINDEX] = NLA_POLICY_MIN(NLA_S32, 0),
>  };
>  
>  static const struct genl_small_ops dp_datapath_genl_ops[] = {
> @@ -2302,7 +2302,7 @@ static int ovs_vport_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  	parms.port_no = port_no;
>  	parms.upcall_portids = a[OVS_VPORT_ATTR_UPCALL_PID];
>  	parms.desired_ifindex = a[OVS_VPORT_ATTR_IFINDEX]
> -		? nla_get_u32(a[OVS_VPORT_ATTR_IFINDEX]) : 0;
> +		? nla_get_s32(a[OVS_VPORT_ATTR_IFINDEX]) : 0;
>  
>  	vport = new_vport(&parms);
>  	err = PTR_ERR(vport);
> @@ -2539,7 +2539,7 @@ static const struct nla_policy vport_policy[OVS_VPORT_ATTR_MAX + 1] = {
>  	[OVS_VPORT_ATTR_TYPE] = { .type = NLA_U32 },
>  	[OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_UNSPEC },
>  	[OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
> -	[OVS_VPORT_ATTR_IFINDEX] = { .type = NLA_U32 },
> +	[OVS_VPORT_ATTR_IFINDEX] = NLA_POLICY_MIN(NLA_S32, 0),
>  	[OVS_VPORT_ATTR_NETNSID] = { .type = NLA_S32 },
>  	[OVS_VPORT_ATTR_UPCALL_STATS] = { .type = NLA_NESTED },
>  };