From: Aaron Conole <aconole@redhat•com>
To: Jakub Kicinski <kuba@kernel•org>
Cc: davem@davemloft•net, andrey.zhadchenko@virtuozzo•com,
dev@openvswitch•org, brauner@kernel•org,
netdev@vger•kernel.org, edumazet@google•com, pabeni@redhat•com,
syzbot+7456b5dcf65111553320@syzkaller•appspotmail.com
Subject: Re: [ovs-dev] [PATCH net] net: openvswitch: reject negative ifindex
Date: Wed, 16 Aug 2023 08:05:41 -0400 [thread overview]
Message-ID: <f7tsf8jxkdm.fsf@redhat.com> (raw)
In-Reply-To: <20230815191809.2d18c9f5@kernel.org> (Jakub Kicinski's message of "Tue, 15 Aug 2023 19:18:09 -0700")
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.
next prev parent reply other threads:[~2023-08-16 12:05 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-14 20:38 [PATCH net] net: openvswitch: reject negative ifindex Jakub Kicinski
2023-08-15 7:01 ` Leon Romanovsky
2023-08-15 12:41 ` [ovs-dev] " Aaron Conole
2023-08-16 2:18 ` Jakub Kicinski
2023-08-16 12:05 ` Aaron Conole [this message]
2023-08-16 2:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f7tsf8jxkdm.fsf@redhat.com \
--to=aconole@redhat$(echo .)com \
--cc=andrey.zhadchenko@virtuozzo$(echo .)com \
--cc=brauner@kernel$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=dev@openvswitch$(echo .)org \
--cc=edumazet@google$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=syzbot+7456b5dcf65111553320@syzkaller$(echo .)appspotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox