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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230815191809.2d18c9f5@kernel.org>
Date: Tue, 15 Aug 2023 19:18:09 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Aaron Conole <aconole@...hat.com>
Cc: davem@...emloft.net, andrey.zhadchenko@...tuozzo.com,
 dev@...nvswitch.org, brauner@...nel.org, netdev@...r.kernel.org,
 edumazet@...gle.com, pabeni@...hat.com,
 syzbot+7456b5dcf65111553320@...kaller.appspotmail.com
Subject: 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": "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@...kaller.appspotmail.com
> > Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> > ---
> > CC: pshelar@....org
> > CC: andrey.zhadchenko@...tuozzo.com
> > CC: brauner@...nel.org
> > CC: dev@...nvswitch.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...