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

[Aaron Conole <aconole@redhat.com>]

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.