[PATCH] net: rtnl: avoid uninitialized data in IFLA_VF_VLAN_LIST handling

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

On Fri, Sep 30, 2016 at 9:13 AM, Arnd Bergmann [off-list ref] wrote:

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.

On Friday 30 September 2016, Eric Dumazet wrote:

quoted

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

On Fri, Sep 30, 2016 at 7:13 PM, Arnd Bergmann [off-list ref] 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>

Hi Arnd,


On 30/09/2016 7:38 PM, Arnd Bergmann wrote:

On Friday 30 September 2016, Eric Dumazet wrote:

quoted

quoted

@@ -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 <redacted>

Thanks for the fix, I will add the flag to my makefile.

quoted

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

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.