Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
Jakub Kicinski 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": "0001", "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+7456b5dcf65111553...@syzkaller.appspotmail.com >> > Signed-off-by: Jakub Kicinski >> > --- >> > CC: pshe...@ovn.org >> > CC: andrey.zhadche...@virtuozzo.com >> > CC: brau...@kernel.org >> > CC: d...@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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
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": "0001", "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+7456b5dcf65111553...@syzkaller.appspotmail.com > > Signed-off-by: Jakub Kicinski > > --- > > CC: pshe...@ovn.org > > CC: andrey.zhadche...@virtuozzo.com > > CC: brau...@kernel.org > > CC: d...@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... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski : 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": "0001", "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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
Jakub Kicinski 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": "0001", "name": "some-port0", > "dp-ifindex":3,"ifindex":4294901760,"type":2}' > > $ ip link show > -65536: some-port0: 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": "0001", "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+7456b5dcf65111553...@syzkaller.appspotmail.com > Signed-off-by: Jakub Kicinski > --- > CC: pshe...@ovn.org > CC: andrey.zhadche...@virtuozzo.com > CC: brau...@kernel.org > CC: d...@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 > 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(); > 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 }, > }; ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
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": "0001", "name": "some-port0", > "dp-ifindex":3,"ifindex":4294901760,"type":2}' > > $ ip link show > -65536: some-port0: 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": "0001", "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+7456b5dcf65111553...@syzkaller.appspotmail.com > Signed-off-by: Jakub Kicinski > --- > CC: pshe...@ovn.org > CC: andrey.zhadche...@virtuozzo.com > CC: brau...@kernel.org > CC: d...@openvswitch.org > --- > net/openvswitch/datapath.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > Thanks, Reviewed-by: Leon Romanovsky ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
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": "0001", "name": "some-port0", "dp-ifindex":3,"ifindex":4294901760,"type":2}' $ ip link show -65536: some-port0: 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": "0001", "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+7456b5dcf65111553...@syzkaller.appspotmail.com Signed-off-by: Jakub Kicinski --- CC: pshe...@ovn.org CC: andrey.zhadche...@virtuozzo.com CC: brau...@kernel.org CC: d...@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(); 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev