net/ipv6: Do not allow route add with a device that is down
Subsystems (3)
PATCH v2 1/1 net/ipv6: Do not allow route add with a device that is down
IPv6 allows routes to be installed when the device is not up (admin up). Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really there is no reason for IPv6 to allow it, so check the flags and deny if device is admin down.
---
v2
- missed setting err to -ENETDOWN (thanks for catching that Roopa)
net/ipv6/route.c | 6 ++++++ 1 file changed, 6 insertions(+)
net/ipv6/route.c +6 -0
@@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,if (!dev) goto out; + err = -ENETDOWN ; +if (!(dev->flags &IFF_UP )) { +NL_SET_ERR_MSG (extack,"Nexthop device is not up" ); + goto out; + } +if (!ipv6_addr_any (&cfg->fc_prefsrc)) {if (!ipv6_chk_addr (net, &cfg->fc_prefsrc, dev,0 )) {NL_SET_ERR_MSG (extack,"Invalid source address" );
Signature · 1 line
2.11.0
Quoted 20 lines · On Wed, 24 Jan 2018 08:29:24 -0800 David Ahern <[email protected]> wrote:
IPv6 allows routes to be installed when the device is not up (admin up). Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really there is no reason for IPv6 to allow it, so check the flags and deny if device is admin down.
Signed-off-by David Ahern <[email protected]>---
v2
- missed setting err to-ENETDOWN(thanks for catching that Roopa)net/ipv6/route.c | 6 ++++++ 1 file changed, 6 insertions(+)net/ipv6/route.c +0
-0@@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,if (!dev) goto out;
net/ipv6/route.c static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg, · line 2745
Quoted 10 lines
net/ipv6/route.c +6
-0+ err = -ENETDOWN ; +if (!(dev->flags &IFF_UP )) { +NL_SET_ERR_MSG (extack,"Nexthop device is not up" ); + goto out; + } +if (!ipv6_addr_any (&cfg->fc_prefsrc)) {if (!ipv6_chk_addr (net, &cfg->fc_prefsrc, dev,0 )) {NL_SET_ERR_MSG (extack,"Invalid source address" );
This looks like a good idea.
There are two equal ways to check for admin up. Either the dev flags or look at link state via netif_running(). Maybe the latter would be better.
Quoted 36 lines · Stephen Hemminger
Quoted 20 lines · On Wed, 24 Jan 2018 08:29:24 -0800 David Ahern <[email protected]> wrote:
IPv6 allows routes to be installed when the device is not up (admin up). Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really there is no reason for IPv6 to allow it, so check the flags and deny if device is admin down.
Signed-off-by David Ahern <[email protected]>---
v2
- missed setting err to-ENETDOWN(thanks for catching that Roopa)net/ipv6/route.c | 6 ++++++ 1 file changed, 6 insertions(+)net/ipv6/route.c +0
-0@@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,if (!dev) goto out;net/ipv6/route.c static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg, · line 2745
Quoted 10 lines
net/ipv6/route.c +6
-0+ err = -ENETDOWN ; +if (!(dev->flags &IFF_UP )) { +NL_SET_ERR_MSG (extack,"Nexthop device is not up" ); + goto out; + } +if (!ipv6_addr_any (&cfg->fc_prefsrc)) {if (!ipv6_chk_addr (net, &cfg->fc_prefsrc, dev,0 )) {NL_SET_ERR_MSG (extack,"Invalid source address" );This looks like a good idea.
Quoted 4 lines
There are two equal ways to check for admin up. Either the dev flags or look at link state via
netif_running(). Maybe the latter would be better.
I used dev->flags for consistency with IPv4. Looking at use of netif_running vs dev->flags in IPv4 and IPv6 code the flag is much more prevalent.
Quoted 18 lines · David Ahern
IPv6 allows routes to be installed when the device is not up (admin up). Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really there is no reason for IPv6 to allow it, so check the flags and deny if device is admin down.
Signed-off-by David Ahern <[email protected]>---
v2
- missed setting err to-ENETDOWN(thanks for catching that Roopa)net/ipv6/route.c | 6 ++++++ 1 file changed, 6 insertions(+)net/ipv6/route.c +0
-0
net/ipv6/route.c static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg, · line 2741
Quoted 8 lines
net/ipv6/route.c +5
-0@@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,if (!dev) goto out; + err = -ENETDOWN ; +if (!(dev->flags &IFF_UP )) { +NL_SET_ERR_MSG (extack,"Nexthop device is not up" ); + goto out; + }
Note that since you have to take a branch, you can move the err = -ENETDOWN inside the branch.
The trick of setting err before doing :
if (cond)
goto xxxx;
Is only relevant if the goto is naked.
By looking at your patch without more context, it is not clear that the "err = -ENETDOWN" has no side effect.
Quoted 32 lines · Eric Dumazet
Quoted 18 lines · David Ahern
IPv6 allows routes to be installed when the device is not up (admin up). Worse, it does not mark it as LINKDOWN. IPv4 does not allow it and really there is no reason for IPv6 to allow it, so check the flags and deny if device is admin down.
Signed-off-by David Ahern <[email protected]>---
v2
- missed setting err to-ENETDOWN(thanks for catching that Roopa)net/ipv6/route.c | 6 ++++++ 1 file changed, 6 insertions(+)net/ipv6/route.c +0
-0net/ipv6/route.c static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg, · line 2741
Quoted 8 lines
net/ipv6/route.c +5
-0@@ -2734,6 +2734,12 @@ static struct rt6_info *ip6_route_info_create(struct fib6_config *cfg,if (!dev) goto out; + err = -ENETDOWN ; +if (!(dev->flags &IFF_UP )) { +NL_SET_ERR_MSG (extack,"Nexthop device is not up" ); + goto out; + }Note that since you have to take a branch, you can move the err =
-ENETDOWNinside the branch.
Quoted 6 lines
The trick of setting err before doing :
if (cond)
goto xxxx;Is only relevant if the goto is naked.
yes, i was tempted to put it inside the brackets, but recall in the past some reviewers have strong opinions about it being set first. I can move it.
Quoted 4 lines
By looking at your patch without more context, it is not clear that the "err =
-ENETDOWN" has no side effect.
It doesn't. only 1 error path after this code and it sets err.