netdev - Re: [iproute PATCH] ip-route: Propagate errors from parse_one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Jan 2018 10:19:24 +0100
From:   Phil Sutter <phil@....cc>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org,
        Élie Bouttier <elie@...ttier.eu>
Subject: Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

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@....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@...hat.com>
> > Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
> > Signed-off-by: Phil Sutter <phil@....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