On Tue, 23 Jan 2018 17:40:47 +0100
Phil Sutter <p...@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 <lpoet...@redhat.com>
> Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
> Signed-off-by: Phil Sutter <p...@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.
