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

[Jakub Kicinski <kuba@kernel.org>]

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