[iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Phil Sutter]

The following command segfaults if enp0s31f6 does not exist:

| # ip -6 route add default proto ra metric 20100 \
| 	nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \
| 	nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1

Since the non-zero return code from parse_one_nh() is ignored,
parse_nexthops() continues iterating over the the same fields in argv
until buffer space is exhausted and eventually accesses unallocated
memory.

Fix this by aborting on error in parse_nexthops() and make
iproute_modify() fail if parse_nexthops() did.

Reported-by: Lennart Poettering <lpoetter@redhat.com>
Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iproute.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ip/iproute.c b/ip/iproute.c
index bf886fda9d761..d7accf57ac8d1 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r,
 		memset(rtnh, 0, sizeof(*rtnh));
 		rtnh->rtnh_len = sizeof(*rtnh);
 		rta->rta_len += rtnh->rtnh_len;
-		parse_one_nh(n, r, rta, rtnh, &argc, &argv);
+		if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0)
+			return -1;
 		rtnh = RTNH_NEXT(rtnh);
 	}
 
@@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
 		addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), RTA_PAYLOAD(mxrta));
 	}
 
-	if (nhs_ok)
-		parse_nexthops(&req.n, &req.r, argc, argv);
+	if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0)
+		return -1;
 
 	if (req.r.rtm_family == AF_UNSPEC)
 		req.r.rtm_family = AF_INET;
-- 
2.15.1

Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Stephen Hemminger]

On Tue, 23 Jan 2018 17:40:47 +0100
Phil Sutter <phil@nwl.cc> wrote:

> The following command segfaults if enp0s31f6 does not exist:
> 
> | # ip -6 route add default proto ra metric 20100 \
> | 	nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \
> | 	nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1
> 
> Since the non-zero return code from parse_one_nh() is ignored,
> parse_nexthops() continues iterating over the the same fields in argv
> until buffer space is exhausted and eventually accesses unallocated
> memory.
> 
> Fix this by aborting on error in parse_nexthops() and make
> iproute_modify() fail if parse_nexthops() did.
> 
> Reported-by: Lennart Poettering <lpoetter@redhat.com>
> Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  ip/iproute.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ip/iproute.c b/ip/iproute.c
> index bf886fda9d761..d7accf57ac8d1 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r,
>  		memset(rtnh, 0, sizeof(*rtnh));
>  		rtnh->rtnh_len = sizeof(*rtnh);
>  		rta->rta_len += rtnh->rtnh_len;
> -		parse_one_nh(n, r, rta, rtnh, &argc, &argv);
> +		if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0)
> +			return -1;
>  		rtnh = RTNH_NEXT(rtnh);
>  	}
>  
> @@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
>  		addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), RTA_PAYLOAD(mxrta));
>  	}
>  
> -	if (nhs_ok)
> -		parse_nexthops(&req.n, &req.r, argc, argv);
> +	if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0)
> +		return -1;
>  
>  	if (req.r.rtm_family == AF_UNSPEC)
>  		req.r.rtm_family = AF_INET;


The real issue is that handling of invalid device is different than all the other
possible semantic errors.

My recommendations are:
	* change bad device to use invarg() which does exit
	* make functions that only return 0 void including
		parse_one_nh
		lwt_parse_encap
		get_addr

Also, it looks like read_family converts any address family it doesn't know about to unspec
that is stupid behavior as well.

The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
looks like well intentioned but suspect. Most of the errors in ip route
indicate real issues where continuing is not a good plan.

		

Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Phil Sutter]

Hi Stephen,

On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote:
> On Tue, 23 Jan 2018 17:40:47 +0100
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > The following command segfaults if enp0s31f6 does not exist:
> > 
> > | # ip -6 route add default proto ra metric 20100 \
> > | 	nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \
> > | 	nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1
> > 
> > Since the non-zero return code from parse_one_nh() is ignored,
> > parse_nexthops() continues iterating over the the same fields in argv
> > until buffer space is exhausted and eventually accesses unallocated
> > memory.
> > 
> > Fix this by aborting on error in parse_nexthops() and make
> > iproute_modify() fail if parse_nexthops() did.
> > 
> > Reported-by: Lennart Poettering <lpoetter@redhat.com>
> > Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  ip/iproute.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ip/iproute.c b/ip/iproute.c
> > index bf886fda9d761..d7accf57ac8d1 100644
> > --- a/ip/iproute.c
> > +++ b/ip/iproute.c
> > @@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r,
> >  		memset(rtnh, 0, sizeof(*rtnh));
> >  		rtnh->rtnh_len = sizeof(*rtnh);
> >  		rta->rta_len += rtnh->rtnh_len;
> > -		parse_one_nh(n, r, rta, rtnh, &argc, &argv);
> > +		if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0)
> > +			return -1;
> >  		rtnh = RTNH_NEXT(rtnh);
> >  	}
> >  
> > @@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
> >  		addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), RTA_PAYLOAD(mxrta));
> >  	}
> >  
> > -	if (nhs_ok)
> > -		parse_nexthops(&req.n, &req.r, argc, argv);
> > +	if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0)
> > +		return -1;
> >  
> >  	if (req.r.rtm_family == AF_UNSPEC)
> >  		req.r.rtm_family = AF_INET;
> 
> 
> The real issue is that handling of invalid device is different than all the other
> possible semantic errors.
> 
> My recommendations are:
> 	* change bad device to use invarg() which does exit
> 	* make functions that only return 0 void including
> 		parse_one_nh
> 		lwt_parse_encap
> 		get_addr
> 
> Also, it looks like read_family converts any address family it doesn't know about to unspec
> that is stupid behavior as well.
> 
> The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
> looks like well intentioned but suspect. Most of the errors in ip route
> indicate real issues where continuing is not a good plan.

You're right, the use of invarg() for any other error effectively
prevents what said commit tried to achieve, so my fix is pretty
pointless in that regard. Yet I wonder why we still have 'ip -batch
-force' given that it's not useful. Maybe Élie is able to provide some
details about the use-case said commit tried to fix?

Meanwhile I'll prepare some patches to address the shortcomings you
mentioned above.

Thanks, Phil

Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Phil Sutter]

On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote:
[...]
> Also, it looks like read_family converts any address family it doesn't know about to unspec
> that is stupid behavior as well.

I had a closer look and it is the best thing it could do. In all but one
cases, the function is called to parse an optional family spec so
parsing failure is expected behaviour.

Cheers, Phil

Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Stephen Hemminger]

On Wed, 24 Jan 2018 10:19:24 +0100
Phil Sutter <phil@nwl.cc> wrote:

> Hi Stephen,
> 
> On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote:
> > On Tue, 23 Jan 2018 17:40:47 +0100
> > Phil Sutter <phil@nwl.cc> wrote:
> >   
> > > The following command segfaults if enp0s31f6 does not exist:
> > > 
> > > | # ip -6 route add default proto ra metric 20100 \
> > > | 	nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \
> > > | 	nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1
> > > 
> > > Since the non-zero return code from parse_one_nh() is ignored,
> > > parse_nexthops() continues iterating over the the same fields in argv
> > > until buffer space is exhausted and eventually accesses unallocated
> > > memory.
> > > 
> > > Fix this by aborting on error in parse_nexthops() and make
> > > iproute_modify() fail if parse_nexthops() did.
> > > 
> > > Reported-by: Lennart Poettering <lpoetter@redhat.com>
> > > Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  ip/iproute.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/ip/iproute.c b/ip/iproute.c
> > > index bf886fda9d761..d7accf57ac8d1 100644
> > > --- a/ip/iproute.c
> > > +++ b/ip/iproute.c
> > > @@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r,
> > >  		memset(rtnh, 0, sizeof(*rtnh));
> > >  		rtnh->rtnh_len = sizeof(*rtnh);
> > >  		rta->rta_len += rtnh->rtnh_len;
> > > -		parse_one_nh(n, r, rta, rtnh, &argc, &argv);
> > > +		if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0)
> > > +			return -1;
> > >  		rtnh = RTNH_NEXT(rtnh);
> > >  	}
> > >  
> > > @@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
> > >  		addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), RTA_PAYLOAD(mxrta));
> > >  	}
> > >  
> > > -	if (nhs_ok)
> > > -		parse_nexthops(&req.n, &req.r, argc, argv);
> > > +	if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0)
> > > +		return -1;
> > >  
> > >  	if (req.r.rtm_family == AF_UNSPEC)
> > >  		req.r.rtm_family = AF_INET;  
> > 
> > 
> > The real issue is that handling of invalid device is different than all the other
> > possible semantic errors.
> > 
> > My recommendations are:
> > 	* change bad device to use invarg() which does exit
> > 	* make functions that only return 0 void including
> > 		parse_one_nh
> > 		lwt_parse_encap
> > 		get_addr
> > 
> > Also, it looks like read_family converts any address family it doesn't know about to unspec
> > that is stupid behavior as well.
> > 
> > The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
> > looks like well intentioned but suspect. Most of the errors in ip route
> > indicate real issues where continuing is not a good plan.  
> 
> You're right, the use of invarg() for any other error effectively
> prevents what said commit tried to achieve, so my fix is pretty
> pointless in that regard. Yet I wonder why we still have 'ip -batch
> -force' given that it's not useful. Maybe Élie is able to provide some
> details about the use-case said commit tried to fix?
> 
> Meanwhile I'll prepare some patches to address the shortcomings you
> mentioned above.

The use case for batch (and force) is that there may be a large set of routes
or qdisc operations where it is ok for some of them to fail because of responses
from the kernel failing.  I don't think batch should ever just continue if handed
invalid syntax for device or address. There are some borderline cases, for example
if a tunnel device could not be created and later steps depend on that name.

Agree, lets get some real data on why the original patch was done.

Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Élie Bouttier]

On 24/01/2018 16:44, Stephen Hemminger wrote:
> On Wed, 24 Jan 2018 10:19:24 +0100
> Phil Sutter <phil@nwl.cc> wrote:
> 
>> Hi Stephen,
>>
>> On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote:
>>> On Tue, 23 Jan 2018 17:40:47 +0100
>>> Phil Sutter <phil@nwl.cc> wrote:
>>>   
>>>> The following command segfaults if enp0s31f6 does not exist:
>>>>
>>>> | # ip -6 route add default proto ra metric 20100 \
>>>> | 	nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \
>>>> | 	nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1
>>>>
>>>> Since the non-zero return code from parse_one_nh() is ignored,
>>>> parse_nexthops() continues iterating over the the same fields in argv
>>>> until buffer space is exhausted and eventually accesses unallocated
>>>> memory.
>>>>
>>>> Fix this by aborting on error in parse_nexthops() and make
>>>> iproute_modify() fail if parse_nexthops() did.
>>>>
>>>> Reported-by: Lennart Poettering <lpoetter@redhat.com>
>>>> Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
>>>> Signed-off-by: Phil Sutter <phil@nwl.cc>
>>>> ---
>>>>  ip/iproute.c | 7 ++++---
>>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/ip/iproute.c b/ip/iproute.c
>>>> index bf886fda9d761..d7accf57ac8d1 100644
>>>> --- a/ip/iproute.c
>>>> +++ b/ip/iproute.c
>>>> @@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r,
>>>>  		memset(rtnh, 0, sizeof(*rtnh));
>>>>  		rtnh->rtnh_len = sizeof(*rtnh);
>>>>  		rta->rta_len += rtnh->rtnh_len;
>>>> -		parse_one_nh(n, r, rta, rtnh, &argc, &argv);
>>>> +		if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0)
>>>> +			return -1;
>>>>  		rtnh = RTNH_NEXT(rtnh);
>>>>  	}
>>>>  
>>>> @@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
>>>>  		addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), RTA_PAYLOAD(mxrta));
>>>>  	}
>>>>  
>>>> -	if (nhs_ok)
>>>> -		parse_nexthops(&req.n, &req.r, argc, argv);
>>>> +	if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0)
>>>> +		return -1;
>>>>  
>>>>  	if (req.r.rtm_family == AF_UNSPEC)
>>>>  		req.r.rtm_family = AF_INET;  
>>>
>>>
>>> The real issue is that handling of invalid device is different than all the other
>>> possible semantic errors.
>>>
>>> My recommendations are:
>>> 	* change bad device to use invarg() which does exit
>>> 	* make functions that only return 0 void including
>>> 		parse_one_nh
>>> 		lwt_parse_encap
>>> 		get_addr
>>>
>>> Also, it looks like read_family converts any address family it doesn't know about to unspec
>>> that is stupid behavior as well.
>>>
>>> The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
>>> looks like well intentioned but suspect. Most of the errors in ip route
>>> indicate real issues where continuing is not a good plan.  
>>
>> You're right, the use of invarg() for any other error effectively
>> prevents what said commit tried to achieve, so my fix is pretty
>> pointless in that regard. Yet I wonder why we still have 'ip -batch
>> -force' given that it's not useful. Maybe Élie is able to provide some
>> details about the use-case said commit tried to fix?
>>
>> Meanwhile I'll prepare some patches to address the shortcomings you
>> mentioned above.
> 
> The use case for batch (and force) is that there may be a large set of routes
> or qdisc operations where it is ok for some of them to fail because of responses
> from the kernel failing.  I don't think batch should ever just continue if handed
> invalid syntax for device or address. There are some borderline cases, for example
> if a tunnel device could not be created and later steps depend on that name.
> 
> Agree, lets get some real data on why the original patch was done.

If I remember correctly, I made this commit for a batch of "route get"
and not stop on inexistent routes. But my patch was not limited to this
use case and I tried to fix others potential situations where we should
not stop. The change I made in parse_one_nh is not directly related to
my use care, sorry for the introduced regression, I should have been
more careful.

Ihmo, if a tunnel device could not be created, later steps depending on
it will fail too, but we potentially still want subsequent operations
(for instance the creation of a second tunnel) to be done. I you don't
want it, don't use -force or make two batch files. From this point of
view, Phil patch is better than invarg() but I'm fine with invarg() too.


Thank you all and sorry again,

Élie

Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

[Phil Sutter]

Hi,

On Thu, Feb 08, 2018 at 02:26:05PM +0100, Élie Bouttier wrote:
> On 24/01/2018 16:44, Stephen Hemminger wrote:
> > On Wed, 24 Jan 2018 10:19:24 +0100
> > Phil Sutter <phil@nwl.cc> wrote:
> >> On Tue, Jan 23, 2018 at 02:44:42PM -0800, Stephen Hemminger wrote:
> >>> The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
> >>> looks like well intentioned but suspect. Most of the errors in ip route
> >>> indicate real issues where continuing is not a good plan.  
> >>
> >> You're right, the use of invarg() for any other error effectively
> >> prevents what said commit tried to achieve, so my fix is pretty
> >> pointless in that regard. Yet I wonder why we still have 'ip -batch
> >> -force' given that it's not useful. Maybe Élie is able to provide some
> >> details about the use-case said commit tried to fix?
> >>
> >> Meanwhile I'll prepare some patches to address the shortcomings you
> >> mentioned above.
> > 
> > The use case for batch (and force) is that there may be a large set of routes
> > or qdisc operations where it is ok for some of them to fail because of responses
> > from the kernel failing.  I don't think batch should ever just continue if handed
> > invalid syntax for device or address. There are some borderline cases, for example
> > if a tunnel device could not be created and later steps depend on that name.
> > 
> > Agree, lets get some real data on why the original patch was done.
> 
> If I remember correctly, I made this commit for a batch of "route get"
> and not stop on inexistent routes. But my patch was not limited to this
> use case and I tried to fix others potential situations where we should
> not stop. The change I made in parse_one_nh is not directly related to
> my use care, sorry for the introduced regression, I should have been
> more careful.
> 
> Ihmo, if a tunnel device could not be created, later steps depending on
> it will fail too, but we potentially still want subsequent operations
> (for instance the creation of a second tunnel) to be done. I you don't
> want it, don't use -force or make two batch files. From this point of
> view, Phil patch is better than invarg() but I'm fine with invarg() too.

OK, that 'route get' case seems pretty valid for me. Also, the situation
I was fixing for in the first place was caused by non-existing
interface, which may very well be caused by a previous attempt at
creating it which had failed. So not really a case of "invalid syntax",
either.

I guess the best approach to satisfy all considerations would be to make
a clear distinction between syntax errors and others (including
non-existing interface e.g.) and not allowing for the latter to exit the
program but propagate errors up to a non-zero return code in do_cmd().
Might be quite some work given that '-batch -force' seems to be a rarely
used combination.

I had a quick idea of changing batch mode to fork and exec in order to
avoid the diligent work needed, but a quick comparison between batch
mode and separate ip invocations for about 64k commands each showed a
slowdown of over 20 times, so really not an alternative here.

My guess is that distinguishing between syntax error and "regular" ones
won't be very clear in all cases and trying to maintain it might be
error-prone. So I'd personally just not make that distinction at all but
try to let -force continue at all times, no matter what. Especially
since the user explicitly requested this behaviour.

Stephen, I think this is a maintainer's decision now. :)

Cheers, Phil