[PATCH net] net: openvswitch: reject negative ifindex

[PATCH net] net: openvswitch: reject negative ifindex

[Jakub Kicinski]

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:

$ ./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
---
 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 },
 };
-- 
2.41.0

Re: [PATCH net] net: openvswitch: reject negative ifindex

[Leon Romanovsky]

On Mon, Aug 14, 2023 at 01:38:40PM -0700, Jakub Kicinski wrote:
> 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:
> 
> $ ./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
> ---
>  net/openvswitch/datapath.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

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

[Aaron Conole]

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 },
>  };

Re: [PATCH net] net: openvswitch: reject negative ifindex

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Aug 2023 13:38:40 -0700 you wrote:
> 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}'
> 
> [...]

Here is the summary with links:
  - [net] net: openvswitch: reject negative ifindex
    https://git.kernel.org/netdev/net/c/a552bfa16bab

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Jakub Kicinski]

On Tue, 15 Aug 2023 08:41:50 -0400 Aaron Conole wrote:
> > Validate the inputes. Now the second command correctly returns:  
> 
> s/inputes/inputs/

Thanks, fixed when applying

> > $ ./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.

Ah. Syzbot pointed at my commit so I thought others will just think
"not my bug" :)

> 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.

Speaking of fuzzing - reaching out to Dmitry crossed my mind.
When the first netlink specs got merged we briefly discussed
using them to guide syzbot a little. But then I thought - syzbot did
find this fairly quickly, it's more that previously we apparently had
no warning or crash for negative ifindex so there was no target to hit.

> 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.

FWIW I left the nla_put_u32() when outputting ifindex in the kernel as
well. I needed s32 for the range because min and max are 16 bit (to
conserve space in the policy) so I could not express the positive limit
on u32. Whether ifindex is u32 or s32 is a bit of a philosophical
question to me, as it only takes positive 31b values...

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

[Aaron Conole]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 15 Aug 2023 08:41:50 -0400 Aaron Conole wrote:
>> > Validate the inputes. Now the second command correctly returns:  
>> 
>> s/inputes/inputs/
>
> Thanks, fixed when applying
>
>> > $ ./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.
>
> Ah. Syzbot pointed at my commit so I thought others will just think
> "not my bug" :)
>
>> 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.
>
> Speaking of fuzzing - reaching out to Dmitry crossed my mind.
> When the first netlink specs got merged we briefly discussed
> using them to guide syzbot a little. But then I thought - syzbot did
> find this fairly quickly, it's more that previously we apparently had
> no warning or crash for negative ifindex so there was no target to hit.
>
>> 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.
>
> FWIW I left the nla_put_u32() when outputting ifindex in the kernel as
> well.

I looked around and it is a bit inconsistent when sending the ifindex.
Some places are s32, some are u32, and doesn't seem to be any rhyme or
reason other than "feels good here."

> I needed s32 for the range because min and max are 16 bit (to
> conserve space in the policy) so I could not express the positive limit
> on u32. Whether ifindex is u32 or s32 is a bit of a philosophical
> question to me, as it only takes positive 31b values...

Yeah, we can always just use some number > INT_MAX, and we will have the
sign bit as well, so it isn't too important.