* [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
@ 2016-09-30 16:13 Arnd Bergmann
2016-09-30 16:23 ` Eric Dumazet
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Arnd Bergmann @ 2016-09-30 16:13 UTC (permalink / raw)
To: David S. Miller
Cc: Arnd Bergmann, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Eric Dumazet, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, netdev, linux-kernel
With the newly added support for IFLA_VF_VLAN_LIST netlink messages,
we get a warning about potential uninitialized variable use in
the parsing of the user input when enabling the -Wmaybe-uninitialized
warning:
net/core/rtnetlink.c: In function 'do_setvfinfo':
net/core/rtnetlink.c:1756:9: error: 'ivvl$' may be used uninitialized in this function [-Werror=maybe-uninitialized]
I have not been able to prove whether it is possible to arrive in
this code with an empty IFLA_VF_VLAN_LIST block, but if we do,
then ndo_set_vf_vlan gets called with uninitialized arguments.
This adds an explicit check for an empty list, making it obvious
to the reader and the compiler that this cannot happen.
Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
net/core/rtnetlink.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 3ac8946bf244..b06d2f46b83e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
len++;
}
+ if (len == 0)
+ return -EINVAL;
+
err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
ivvl[0]->qos, ivvl[0]->vlan_proto);
if (err < 0)
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
2016-09-30 16:13 [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling Arnd Bergmann
@ 2016-09-30 16:23 ` Eric Dumazet
2016-09-30 16:38 ` Arnd Bergmann
2016-09-30 20:12 ` Or Gerlitz
2016-10-03 5:32 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2016-09-30 16:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, netdev, LKML
On Fri, Sep 30, 2016 at 9:13 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> With the newly added support for IFLA_VF_VLAN_LIST netlink messages,
> we get a warning about potential uninitialized variable use in
> the parsing of the user input when enabling the -Wmaybe-uninitialized
> warning:
>
> net/core/rtnetlink.c: In function 'do_setvfinfo':
> net/core/rtnetlink.c:1756:9: error: 'ivvl$' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> I have not been able to prove whether it is possible to arrive in
> this code with an empty IFLA_VF_VLAN_LIST block, but if we do,
> then ndo_set_vf_vlan gets called with uninitialized arguments.
>
> This adds an explicit check for an empty list, making it obvious
> to the reader and the compiler that this cannot happen.
>
> Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> net/core/rtnetlink.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 3ac8946bf244..b06d2f46b83e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
>
> len++;
> }
> + if (len == 0)
> + return -EINVAL;
> +
> err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> ivvl[0]->qos, ivvl[0]->vlan_proto);
> if (err < 0)
> --
> 2.9.0
>
So, if I read this code, we build an array, but call ndo_set_vf_vlan()
only using first element ?
Looks like the bug should be fixed in a different way.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
2016-09-30 16:23 ` Eric Dumazet
@ 2016-09-30 16:38 ` Arnd Bergmann
2016-10-02 9:03 ` Tariq Toukan
0 siblings, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2016-09-30 16:38 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S. Miller, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, netdev, LKML, Moshe Shemesh,
Tariq Toukan
On Friday 30 September 2016, Eric Dumazet wrote:
> > @@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
> >
> > len++;
> > }
> > + if (len == 0)
> > + return -EINVAL;
> > +
> > err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
> > ivvl[0]->qos, ivvl[0]->vlan_proto);
> > if (err < 0)
> > --
> > 2.9.0
> >
>
> So, if I read this code, we build an array, but call ndo_set_vf_vlan()
> only using first element ?
>
> Looks like the bug should be fixed in a different way.
I was wondering about this too, but didn't understand enough about it to say
if it was intentional or not. I just realized that I forgot to add Moshe and Tariq
on Cc (I relied on scripts/get_maintainer.pl, but didn't double-check).
I've added them to Cc now, hope they can clarify this.
Arnd
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
2016-09-30 16:13 [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling Arnd Bergmann
2016-09-30 16:23 ` Eric Dumazet
@ 2016-09-30 20:12 ` Or Gerlitz
2016-10-03 5:32 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Or Gerlitz @ 2016-09-30 20:12 UTC (permalink / raw)
To: Arnd Bergmann, Moshe Shemesh, Tariq Toukan
Cc: David S. Miller, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Eric Dumazet, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, Linux Netdev List,
Linux Kernel
On Fri, Sep 30, 2016 at 7:13 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> With the newly added support for IFLA_VF_VLAN_LIST netlink messages,
> we get a warning about potential uninitialized variable use in
> the parsing of the user input when enabling the -Wmaybe-uninitialized
> warning:
>
> net/core/rtnetlink.c: In function 'do_setvfinfo':
> net/core/rtnetlink.c:1756:9: error: 'ivvl$' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> I have not been able to prove whether it is possible to arrive in
> this code with an empty IFLA_VF_VLAN_LIST block, but if we do,
> then ndo_set_vf_vlan gets called with uninitialized arguments.
>
> This adds an explicit check for an empty list, making it obvious
> to the reader and the compiler that this cannot happen.
>
> Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
Added the authors of the above patch
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
2016-09-30 16:38 ` Arnd Bergmann
@ 2016-10-02 9:03 ` Tariq Toukan
0 siblings, 0 replies; 6+ messages in thread
From: Tariq Toukan @ 2016-10-02 9:03 UTC (permalink / raw)
To: Arnd Bergmann, Eric Dumazet
Cc: David S. Miller, Roopa Prabhu, Nicolas Dichtel,
Nikolay Aleksandrov, Jiri Pirko, Brenden Blanco,
Hannes Frederic Sowa, Nogah Frankel, netdev, LKML, Moshe Shemesh
Hi Arnd,
On 30/09/2016 7:38 PM, Arnd Bergmann wrote:
> On Friday 30 September 2016, Eric Dumazet wrote:
>>> @@ -1753,6 +1753,9 @@ static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
>>>
>>> len++;
>>> }
>>> + if (len == 0)
>>> + return -EINVAL;
>>> +
>>> err = ops->ndo_set_vf_vlan(dev, ivvl[0]->vf, ivvl[0]->vlan,
>>> ivvl[0]->qos, ivvl[0]->vlan_proto);
>>> if (err < 0)
>>> --
>>> 2.9.0
>>>
Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
Thanks for the fix, I will add the flag to my makefile.
>> So, if I read this code, we build an array, but call ndo_set_vf_vlan()
>> only using first element ?
>>
>> Looks like the bug should be fixed in a different way.
> I was wondering about this too, but didn't understand enough about it to say
> if it was intentional or not.
It is intentional.
The change we need here is the option to set the VF vlan protocol, so
our hw will insert either S-TAG or C-TAG per VF. The previous version of
this patch didn't use any array.
Then we got a comment, which makes sense, that the new UAPI interface
should be able to support a future driver that might want to set both
S-TAG and C-TAG per VF.
We could have used a single struct on kernel side instead of a list of
size 1, but this would cause bad/not scalable code in the parsing stage.
As the UAPI is a list, in the parsing stage we thought it is more
correct to use the constant MAX_VLAN_LIST_LEN and iterate as of a
general case, even though currently the list size is one.
> I just realized that I forgot to add Moshe and Tariq
> on Cc (I relied on scripts/get_maintainer.pl, but didn't double-check).
> I've added them to Cc now, hope they can clarify this.
>
> Arnd
Regards,
Moshe and Tariq
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling
2016-09-30 16:13 [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling Arnd Bergmann
2016-09-30 16:23 ` Eric Dumazet
2016-09-30 20:12 ` Or Gerlitz
@ 2016-10-03 5:32 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2016-10-03 5:32 UTC (permalink / raw)
To: arnd
Cc: roopa, nicolas.dichtel, nikolay, jiri, edumazet, bblanco, hannes,
nogahf, netdev, linux-kernel
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 30 Sep 2016 18:13:49 +0200
> With the newly added support for IFLA_VF_VLAN_LIST netlink messages,
> we get a warning about potential uninitialized variable use in
> the parsing of the user input when enabling the -Wmaybe-uninitialized
> warning:
>
> net/core/rtnetlink.c: In function 'do_setvfinfo':
> net/core/rtnetlink.c:1756:9: error: 'ivvl$' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> I have not been able to prove whether it is possible to arrive in
> this code with an empty IFLA_VF_VLAN_LIST block, but if we do,
> then ndo_set_vf_vlan gets called with uninitialized arguments.
>
> This adds an explicit check for an empty list, making it obvious
> to the reader and the compiler that this cannot happen.
>
> Fixes: 79aab093a0b5 ("net: Update API for VF vlan protocol 802.1ad support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-03 5:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-30 16:13 [PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling Arnd Bergmann
2016-09-30 16:23 ` Eric Dumazet
2016-09-30 16:38 ` Arnd Bergmann
2016-10-02 9:03 ` Tariq Toukan
2016-09-30 20:12 ` Or Gerlitz
2016-10-03 5:32 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.